vektra / mockery

A mock code autogenerator for Go
https://vektra.github.io/mockery/
BSD 3-Clause "New" or "Revised" License
5.8k stars 395 forks source link

Incorrect mocks generated when there are multiple interfaces in the same package #778

Closed CoderCoco closed 1 month ago

CoderCoco commented 1 month ago

Description

Mocks are incorrectly generated when there is an interface with a function that returns another interface and both of these interfaces are in the same package. This leads to issues when trying to use the mocks in tests:

persistent := args.NewFlags(t)
flags := args.NewFlags(t)

command := args.NewCommand(t)

// This doesn't work
command.EXPECT().Flags().Return(flags)

// This doesn't work
command.Flags().EXPECT().GetBool(mock.Anything).Return(true, nil)
./flags_test.go:38:34: cannot use flags (variable of type *"args".Flags) as "mocks/args".Flags value in argument to command.EXPECT().Flags().Return
./flags_test.go:39:18: cannot call pointer method EXPECT on "args".Flags

Mockery Version

2.43.0

Golang Version

1.22.3

Installation Method

Steps to Reproduce

Create a package with both of these interfaces

package args

type Flags interface {
    GetBool(name string) (bool, error)
    GetString(name string) (string, error)
    BoolP(name, shorthand string, value bool, usage string) *bool
    StringP(name, shorthand string, value string, usage string) *string
}

type Command interface {
    MarkFlagRequired(string) error
    Flags() Flags
    PersistentFlags() Flags
}

Use this config

with-expecter: True
inpackage: True
dir: mocks/{{ replaceAll .InterfaceDirRelative "internal" "internal_" }}
mockname: "{{.InterfaceName}}"
outpkg: "{{.PackageName}}"
filename: "{{.InterfaceName}}.go"
all: true
packages:
  "_pathreplaced_/args":
      config:
        all: false
      interfaces:
        Command:
        Flags:

Expected Behavior

I'm not sure if this is the correct way to solve the issue

// Code generated by mockery v2.43.0. DO NOT EDIT.

package args

import mock "github.com/stretchr/testify/mock"

// Command is an autogenerated mock type for the Command type
type Command struct {
    mock.Mock
}

// Flags provides a mock function with given fields:
func (_m *Command) Flags() *Flags { // <----------- This should be a pointer to the other mocked struct
    // Generated logic
}

Actual Behavior

// Code generated by mockery v2.43.0. DO NOT EDIT.

package args

import mock "github.com/stretchr/testify/mock"

// Command is an autogenerated mock type for the Command type
type Command struct {
    mock.Mock
}

// Flags provides a mock function with given fields:
func (_m *Command) Flags() Flags {
    ret := _m.Called()

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

    var r0 Flags
    if rf, ok := ret.Get(0).(func() Flags); ok {
        r0 = rf()
    } else {
        if ret.Get(0) != nil {
            r0 = ret.Get(0).(Flags)
        }
    }

    return r0
}
CoderCoco commented 1 month ago

I also want to point out that I have tried dereferencing the pointer (see below)

image

But I don't think there's a way for this to be done by value instead of reference. When executing my test I always receive:

// Attempt 1
var f = testMock.command.Command.Flags()
(&f).EXPECT().BoolP(flagName, "", false, "").Return(new(bool))

flag := NewFlag[bool](flagName, testMock.command)

// Attempt 2
testMock.flags.EXPECT().BoolP(flagName, "", false, "").Return(new(bool))
flag := NewFlag[bool](flagName, testMock.command)

Both result in the following error

assert: mock: I don't know what to return because the method call was unexpected.
            Either do Mock.On("BoolP").Return(...) first, or remove the BoolP() call.
LandonTClipp commented 1 month ago

Hi, the config you've specified is not what you want.

with-expecter: True
inpackage: True
dir: mocks/{{ replaceAll .InterfaceDirRelative "internal" "internal_" }}
mockname: "{{.InterfaceName}}"
outpkg: "{{.PackageName}}"
filename: "{{.InterfaceName}}.go"
all: true

You've specified inpackage: True which tells mockery that you're intending to place the mocks in the same location as the original interface. However, your dir parameter has placed it in a separate location. Therefore, mockery is not importing your original package as you expect because of inpackage: True. So in this case, it references Flags as the return value, which it intends to be the original interface, but instead it accidentally resolves to the Flag mock you've created.

CoderCoco commented 1 month ago

Ah I see the issue:

I had gotten this config from this page of the documentation: https://vektra.github.io/mockery/latest/migrating_to_packages/#examples

Separate mocks/ directory[¶](https://vektra.github.io/mockery/latest/migrating_to_packages/#separate-mocks-directory)
Take for example a configuration where you are specifying all: true at the top of your repo and you're placing your mocks in a separate mocks/ directory, mirroring the directory structure of your original repo.

YAML

testonly: False
with-expecter: True
keeptree: True
all: True
The equivalent config for packages looks like this:

YAML

with-expecter: True
inpackage: True
dir: mocks/{{ replaceAll .InterfaceDirRelative "internal" "internal_" }} 
mockname: "{{.InterfaceName}}"
outpkg: "{{.PackageName}}"
filename: "{{.InterfaceName}}.go"
all: True
packages:
  github.com/org/repo:
    config:
      recursive: True
While the example config provided here is more verbose, that is because we're specifying many non-default values in order to retain strict equivalence for this example. It's recommended to refer to the [configuration parameters](https://vektra.github.io/mockery/latest/configuration/#parameter-descriptions) to see the defaults provided.

If this example configuration leads to a bug, at the very least I think the documentation should be changed?

CoderCoco commented 1 month ago

I have changed the config to the following:

with-expecter: True
dir: mocks/{{ replaceAll .InterfaceDirRelative "internal" "internal_" }}
mockname: "{{.InterfaceName}}"
outpkg: "{{.PackageName}}"
filename: "{{.InterfaceName}}.go"
all: true
packages:
  "_pathreplaced_/args":
      config:
        all: false
      interfaces:
        Command:
        Flags:

Which now generates the mock with the correct values, however when I go to run the test I now have a circular dependency because of this structure:

package _path_replaced_/args
    imports _path_replaced_/mocks/args
    imports _path_replaced_/args: import cycle not allowed in test

How would I solve this issue, I tried using replace-type like this

with-expecter: True
dir: mocks/{{ replaceAll .InterfaceDirRelative "internal" "internal_" }}
mockname: "{{.InterfaceName}}"
outpkg: "{{.PackageName}}"
filename: "{{.InterfaceName}}.go"
all: true
packages:
  "_pathreplaced_/args":
      config:
        all: false
      interfaces:
        Command:
           config:
             replace-type:
               - _pathreplaced_/args.Flags=_pathreplaced_/mocks/args.Flags
        Flags:

But that leads to a self import in the mocks package.

Not really sure how to move forward from here. Any suggestions?

LandonTClipp commented 1 month ago

Ah apologies! It seems like the docs are indeed wrong, my bad. I will fix that.

As far as the circular import issue, this is a classic shortcoming of placing the mocks in a separate directory. This is an inherent circular dependency that can only be resolved by placing args.Flags in another package (to break the circular dep) or by placing the mocks in the same package as the interface. The mocks need access to _pathreplaced_/args.Flags but _pathreplaced_/args imports that mock, so what do you do?

https://vektra.github.io/mockery/latest/configuration/#__tabbed_1_2

CoderCoco commented 1 month ago

Ok yeah I must have missed that tab while going over this, still a bit of a go newby lol.

Got it all up and running now.

Thanks for the help.