Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for No Return Value on Generated Mocks #796

Open
1 of 5 tasks
jacknpainter opened this issue Jul 18, 2024 · 5 comments
Open
1 of 5 tasks

Allow for No Return Value on Generated Mocks #796

jacknpainter opened this issue Jul 18, 2024 · 5 comments

Comments

@jacknpainter
Copy link

Description

Sometimes it is necessary for mocked functions to allow for not checking return values. I am currently running into an issue with mocks generated via mockery where I have to specify a .Return value for a mock even though the return value of that mock makes no difference to the running of the functions within the mock.

Mockery Version

mockery v2.43.2

Golang Version

go 1.22.5

Installation Method

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

Steps to Reproduce

  1. Define a helper function to aid with database transactions
    func (s *Service) BeginTxFunc(ctx context.Context, f func(pgx.Tx) error) error {
        if err := pgx.BeginTxFunc(ctx, s.Pool, pgx.TxOptions{}, f); err != nil {
            return err
        }
    
        return nil
    }
  2. Mock this function using mockery, which returns
    // BeginTxFunc provides a mock function with given fields: ctx, f
    func (_m *MockStorage) BeginTxFunc(ctx context.Context, f func(pgx.Tx) error) error {
    	ret := _m.Called(ctx, f)
    
    	if len(ret) == 0 {
    		panic("no return value specified for BeginTxFunc")
    	}
    
    	var r0 error
    	if rf, ok := ret.Get(0).(func(context.Context, func(pgx.Tx) error) error); ok {
    		r0 = rf(ctx, f)
    	} else {
    		r0 = ret.Error(0)
    	}
    
    	return r0
    }
  3. Use function to do something
    func (s *Service) One(ctx context.Context) error {
    	if err := s.storage.BeginTxFunc(ctx, func(pgx.Tx) error{
    		if err := s.storage.Two(ctx, tx); err != nil {
    			return err
    		}
    
    		if err := s.storage.Three(ctx, tx); err != nil {
    			return err
    		}
    	}); err != nil {
    		return err
    	}
    }
  4. Write tests for this function
    m.On("BeginTxFunc", mock.Anything, mock.Anything).Return(nil)
    m.On("Two", mock.Anything, mock.Anything).Return(nil)
    m.On("Three", mock.Anything, mock.Anything).Return(nil)
  5. This test would now fail with the following message
    FAIL: 1 out of 3 expectation(s) were met.
    
    This is due to the return value from BeginTxFunc being used before either of the other two functions can be called

As you can see, the forcing of a .Return value on m.On("BeginTxFunc") stops the test from calling the other two functions as they are nested and the function has already returned.
I have gotten around this problem before by not definining a .Return value when mocking the BeginTxFunc function, but this appears to not be possible via mockery

The line causing me problems specifically in the generated mocks is this

if len(ret) == 0 {
	panic("no return value specified for BeginTxFunc")
}

Expected Behavior

An option to allow mocks without .Return values

OR

An option to override the mock generation for a specific function. For this situation I have found that this works

func (_m *MockStorage) BeginTxFunc(ctx context.Context, f func(pgx.Tx) error) error {
	m.Called(ctx, f)

	return f(nil)
}

Actual Behavior

panic: no return value specified for Xyz
OR
FAIL: 1 out of N expectation(s) were met.

@heiytor
Copy link

heiytor commented Oct 8, 2024

+1 on this

I also think it's beneficial to use mock.Anything when the return value isn't important. For instance, I have a method in the mock that returns an interface to be passed to another mock. Here’s an example:

mock1.On("Method").Return(fakeStruct)
mock2.On("Method", fakeStruct).Return(anotherSomething)

In this case, I don't care about what is returned by mock1.On("Method"), so I believe that reusing mock.Anything or mock.AnythingOfType would be a great solution:

mock1.On("Method").Return(mock.Anything)
mock2.On("Method", mock.Anything).Return(anotherSomething)

@LandonTClipp
Copy link
Collaborator

I think there is a fundamental misunderstanding here of what mocks are. Let's go to your function:

func (s *Service) One(ctx context.Context) error {
	if err := s.storage.BeginTxFunc(ctx, func(pgx.Tx) error{
		if err := s.storage.Two(ctx, tx); err != nil {
			return err
		}

		if err := s.storage.Three(ctx, tx); err != nil {
			return err
		}
	}); err != nil {
		return err
	}
}

You have mocked out s.storage.BeginTxFunc so the anonymous function passed into BeginTxFunc is not being run because BeginTxFunc has been mocked out. That is the fundamental reason why this behaves the way you see, and it's 100% expected behavior.

If you wanted the anonymous function to be run, you have to use something like RunAndReturn and explicitly call the anonymous function that's passed as an argument. https://vektra.github.io/mockery/latest/features/#expecter-structs

As you can see, the forcing of a .Return value on m.On("BeginTxFunc") stops the test from calling the other two functions as they are nested and the function has already returned.

This is indeed not what's happening, as shown above.

As to the broader question of allowing us to not specify a return value, that's unfortunately not something I'm interested in doing. This has been asked before but it creates a further maintenance burden on me to ensure that mockery knows how to always create a zero-value return if nothing was specified (and from a code generation perspective, it's actually not easy to do that for all cases generally). While technically possible, I don't see a real benefit to it. There is a lot of value in being as explicit as possible, and explicitly telling the mock you want it to return a zero-value for your return type is a good thing IMO.

@LandonTClipp
Copy link
Collaborator

LandonTClipp commented Oct 8, 2024

In this case, I don't care about what is returned by mock1.On("Method"), so I believe that reusing mock.Anything or mock.AnythingOfType would be a great solution:

What you're asking for is far more complicated than you might think. How is mockery supposed to know what kind of struct it should create for your returned interface? Should it be an empty struct? A stub? If you're returning an interface, why not just create a mock for it and return the mock?

@heiytor
Copy link

heiytor commented Oct 8, 2024

If you're returning an interface, why not just create a mock for it and return the mock?

While it's possible to mock the first return, doing so adds unnecessary complexity to the test cases.

What you're asking for is far more complicated than you might think.

The mock.Anything (from Testify) is used in the mock.On method of my struct to indicate that the parameter does not need to be asserted. Since this constant is a string with the value mock.Anything, it cannot simply be used in an if statement within the .Return method to not verify the return?

@LandonTClipp
Copy link
Collaborator

Since this constant is a string with the value mock.Anything, it cannot simply be used in an if statement within the .Return method to not verify the return?

Verification of the return value doesn't happen anyway. If your mocked method gets called, mockery has to return something. It has no option. So what does it return? Well, it can return a zero-value, but what does a zero-value look like for an interface? What does that mean? The point is, this is not easy to do for the general case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants