Omitting `.Return()` on an expectation results in a panic and a stack trace rather than a useful error message

[ratkins]

Description

Omitting the .Return() on an expectation results in a panic deep in a transitive dependency that does not indicate what the problem was or 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

When setting an expectation on a function call defined thus:

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

It will look something like:

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

(note missing .Return(nil) at the end of the EXPECT(), an easy mistake to make.)

Expected Behavior

In this situation I would expect an error message saying something like "Required .Return(error) not specified on EXPECT() call for DoThing() invocation on line xxx".

Actual Behavior

I get a panic and a stack trace from a transitive dependency (not mockery) that gives me no idea what the error could possibly be or how to fix it:

  assert: arguments: Cannot call Get(0) because there are 0 argument(s).

  Full Stack Trace
    github.com/stretchr/testify/mock.Arguments.Get(...)
        /Users/ratkins/go/pkg/mod/github.com/stretchr/testify@v1.8.4/mock/mock.go:887
    gitlab.com/xxx/xxx/mocks/gitlab.com/xxx/xxx/src/services/xxx.(*MockAPI).DoThing(0x140000328c0, {0x103229442, 0xb}, 0x0, {0x140003ec700, 0x1, 0x1})
        /Users/ratkins/Developer/xxx/mocks/gitlab.com/xxx/xxx/src/services/xxx/mock_API.go:28 +0x1c8
    gitlab.com/xxx/xxx/src/endpoints/reports_test.glob..func1.1.MakePutHandler.func1(0x140000328c0?)
        /Users/ratkins/Developer/xxx/src/endpoints/reports/reports_put.go:25 +0x1b8
    gitlab.com/xxx/xxx/src/endpoints/reports_test.glob..func1.1()

---

I realise this bug is possibly testify's "fault", but I didn't install testify, I installed mockery, and not being deeply into the Go ecosystem it took me a while to work out what the relationship is.

[LandonTClipp]

This is a totally fair criticism, and I think it'd be easy to fix. Mockery asks testify for the return values using .Called(). The returned list is mock.Arguments. I think we need to add an assertion that the returned argument list is equal to the length of return values, and if it's not, panic with a useful message.

I'll mark this as good-first-issue and hopefully someone will pick it up if I don't get to it.

[mateusmarquezini]

Hi there @LandonTClipp! I'm new here and I'd like to give it a try. If so, can you assign it to me? Thanks!

[LandonTClipp]

Hi @mateusmarquezini, thanks for volunteering. Sure I'll assign to you. If you need any pointers to get you going, even if where to look in the code base, I'm glad to help.

[mateusmarquezini]

Awesome @LandonTClipp! Thank you!

[mateusmarquezini]

Hey @LandonTClipp! I was walking through the code base trying to find where is the exact point where we should fix this, and I guess this is the place, however, the code is generated by mockery so we can't edit it.

So I'm wondering now where the block of code is generated.

I appreciate any help!

[LandonTClipp]

Hi @mateusmarquezini, the code that generates the .Called directive is in this function: https://github.com/vektra/mockery/blob/v2.37.1/pkg/generator.go#L718-L792

The string that contains the _m.Called(... is here: https://github.com/vektra/mockery/blob/v2.37.1/pkg/generator.go#L743

You can see that in the template used to generate the output file, there are two main cases. In one case, the method is checked to see if it has any return values in the signature, as seen here. In the second case, our method has at least one return value, so the _m.Called( string is placed down, and its return value is assigned to the variable RetVariableName (which in the code block you linked to, is simply called ret).

I'm guessing that the solution here will reside 100% within that template string. So it'll be something like:

if len({{ .RetVariableName}}) == 0) {
    // panic with a useful message
}

You'll also want to create a test for this by creating a new "fixture" here: https://github.com/vektra/mockery/tree/v2.37.1/pkg/fixtures, which can be something as simple as

type PanicOnNoReturnValue interface {
    Foo() string
}

Then, direct mockery to generate a mock for this interface in .mockery.yaml (which will output to the mocks/ directory), and create a test that lives next to the fixture you created, whereby you do something like

func TestPanicOnNoReturnValue(t *testing.T) {
    m := mocks.NewPanicOnNoReturnValue(t)
    // Don't specify a return value
    m.EXPECT().Foo()
    // Create a recovery of the panic that we expect: https://go.dev/ref/spec#Handling_panics
    m.Foo() // this should panic
    // assert some things like what the panic message says
}

[mateusmarquezini]

Thank you for your amazing explanation, @LandonTClipp! It's clear now what needs to be done!

Hopefully, I'll submit a PR yet this week.