mockery icon indicating copy to clipboard operation
mockery copied to clipboard

Add github.com/matryer/moq style mocks into mockery

Open LandonTClipp opened this issue 1 year ago • 10 comments

Description

This PR implements a new style parameter that, if set to moq, will generate the matryer/moq-based mocks. The shape of the moq mocks is essentially the same as the original project. I have successfully been able to build a large number of our test fixtures using moq.

Copied over initial files from matryer/moq using https://blog.billyc.io/how-to-copy-one-or-more-files-from-one-git-repo-to-another-and-keep-the-git-history/. I attempted to get Git attribution for any code copied from the moq project, but I may have missed some lines here or there. There were also some minor modifications I did to the moq.templ file to fix some bugs I noticed.

Implements the proposal in #715.

Configuration

quiet: False
disable-version-string: True
with-expecter: True
mockname: "{{.InterfaceName}}Mock"
filename: "{{.InterfaceName}}.go"
dir: "mocks/moq/{{.PackagePath}}"
packages:
  github.com/vektra/mockery/v2/pkg/fixtures:
    config:
      include-regex: '.*'
      exclude-regex: 'RequesterGenerics|UnsafeInterface|requester_unexported'
      style: moq
      outpkg: test
      template-map:
        with-resets: true
        skip-ensure: true
        stub-impl: false

I have elected to make mockery itself agnostic to the template-specific variables inside of moq, like with-resets, skip-ensure etc. These are simply passed through to the template as a black-box map, instead of unmarshalling it into a struct. The reason is because I intend to extend mockery to accept any arbitrarily defined template, so end users would need to pass arbitrary config to their template.

Note that I have excluded some fixtures that were not compiling correctly. I believe these probably couldn't be compiled in the original moq project, although I haven't explicitly checked.

Output Mocks

You can see the moq-generated mocks, starting here: https://github.com/vektra/mockery/pull/725/files#diff-9efb8f179359bcc43fdae6aa075947a1bb80b511bbae81dd82b743939f874bf3

Feature Tracker

This table tracks the features provided in the moq.templ file and the progress made in this PR to plumb the logic through.

name description implemented note
PkgName
Imports
ImportStatement
Multiple mocks per file Adding multiple mock implementations in a single file Adding multiple mocks per file has been determined to warrant a separate PR, as it's a decent amount of work. I know how it will be done, but this PR is already large enough.
Generics Can generate mocks with type constraints ⚠️ generics don't seem to completely work. Some type constraints result in invalid code (like if we use comparable, the generated mock is invalid). Not sure if it doesn't work in original moq project, we should check.
WithResets
SkipEnsure Skip generating the implementation check in the mock file

TODO

  • [x] Implement all variables needed by moq.templ
  • [x] Create mocks for a variety of fixtures
  • [x] Create tests for the generated moq implementations
  • [x] Ensure generics work (check if currently failing generics generation would fail under matryer/moq)

KNOWN BUGS

  • Unable to generate proper imports when a method uses *unsafe.Pointer. The reason is that the unsafe.Pointer type does not match any switch case in the MethodScope.populateImports method. This appears to be an existing bug with moq, so we will not address it here.
  • Mocks with type constraints (generics) tend to fail under certain scenarios. For example, when using comparable, when using a generic interface: see RequesterGeneric fixture.
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/RequesterGenerics.go:16:35: cannot use type comparable outside a type constraint: interface is (or embeds) comparable
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/RequesterGenerics.go:16:92: undefined: TSigned
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/RequesterGenerics.go:40:9: undefined: GenericType
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/UnsafeInterface.go:33:19: undefined: Pointer
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/UnsafeInterface.go:40:9: undefined: Pointer
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/UnsafeInterface.go:47:38: undefined: Pointer
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/UnsafeInterface.go:52:8: undefined: Pointer
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/UnsafeInterface.go:67:7: undefined: Pointer
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/UnsafeInterface.go:70:8: undefined: Pointer
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/requester_unexported.go:12:7: undefined: test
Error: mocks/moq/github.com/vektra/mockery/v2/pkg/fixtures/UnsafeInterface.go:70:8: too many errors (typecheck)

LandonTClipp avatar Oct 19 '23 18:10 LandonTClipp

@breml @sudo-suhas are moq mocks capable of adding multiple mock implementations in a single file? Some parts of the code act as if that is supported, but I wasn't sure. Mockery currently doesn't support this, which is why I ask.

LandonTClipp avatar Nov 21 '23 21:11 LandonTClipp

@LandonTClipp Yes, it is possible to have multiple mock implementations (from different interfaces) in a single file.

given a file:

package file

type Foo interface {
	Bar()
}

type Baz interface {
	Bazer() error
}

the command

moq -fmt goimports -pkg file_test -out file_gen_test.go . Foo Baz

would generate a single file named file_gen_test.go containing the mocks for both interfaces (Foo and Baz).

This is also mentioned in the README.md in the usage section.

The relevant sections in the code:

  • range over the interfaces where mock should be generated for: https://github.com/matryer/moq/blob/main/pkg/moq/moq.go#L60
  • range over the mocks while generating the resulting file in the Go template: https://github.com/matryer/moq/blob/main/internal/template/template.go#L46

breml avatar Nov 21 '23 22:11 breml

Awesome, thanks for clarifying. This will be a bit of a roadblock as mockery has no way to do this currently so I will need to think how I will go about it.

LandonTClipp avatar Nov 21 '23 22:11 LandonTClipp

Codecov Report

Attention: 2127 lines in your changes are missing coverage. Please review.

Comparison is base (8b86cf2) 42.71105% compared to head (8ff0a85) 30.33787%.

Files Patch % Lines
pkg/generator/template_generator.go 0.00000% 152 Missing :warning:
...com/vektra/mockery/v2/pkg/fixtures/expecter_moq.go 0.00000% 145 Missing :warning:
.../mockery/v2/pkg/fixtures/requester_variadic_moq.go 0.00000% 118 Missing :warning:
pkg/registry/registry.go 0.00000% 113 Missing :warning:
pkg/registry/method_scope.go 0.00000% 85 Missing :warning:
...ub.com/vektra/mockery/v2/pkg/fixtures/fooer_moq.go 0.00000% 84 Missing :warning:
...ery/v2/pkg/fixtures/imports_same_as_package_moq.go 0.00000% 76 Missing :warning:
...ktra/mockery/v2/pkg/fixtures/async_producer_moq.go 0.00000% 72 Missing :warning:
pkg/registry/var.go 0.00000% 70 Missing :warning:
...ery/v2/pkg/fixtures/requester_return_elided_moq.go 0.00000% 56 Missing :warning:
... and 42 more
Additional details and impacted files
@@                 Coverage Diff                  @@
##              master        #725          +/-   ##
====================================================
- Coverage   42.71105%   30.33787%   -12.37319%     
====================================================
  Files             63         111          +48     
  Lines           4987        7133        +2146     
====================================================
+ Hits            2130        2164          +34     
- Misses          2657        4763        +2106     
- Partials         200         206           +6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 13 '24 20:02 codecov[bot]

@sudo-suhas @breml

In this PR, we have a mostly working implementation of moq-in-mockery. Please let me know your thoughts, I would really appreciate a thorough review to make sure I haven't missed something important! Thanks.

LandonTClipp avatar Feb 13 '24 20:02 LandonTClipp

Hi @LandonTClipp

I did some initial tests and it looks like I am failing to reproduce my mocks with mockery. Currently I am facing the following issues:

I have a file named adapter_notification.go with an interface NotificationAdapter defined in it. I would like to generate the mock version of this interface in a file called adapter_notification_mock_test.go in the same folder. This leads to two issues:

  • I can not reference the filename of the file where the interface is defined ({{.InterfaceName | snakecase }}_mock_test.go does provide notification_adapter_mock_test.go, but I want to have adapter_notification_mock_test.go, such that the files are alphabetically ordered close to each other).
  • Even with inpackage: True and outpkg: "{{.PackageName}}" set, the generated mock does contain an import for the package where the interface is defined, which obviously leads to an import cycle.

On an other example I tried to generate a mock from a type alias:

type TelemetryClient = appinsights.TelemetryClient

It looks like the type TelemetryClient was not recognized as interface and mockery exits without generating any mock file.

breml avatar Feb 14 '24 20:02 breml

I can not reference the filename of the file where the interface is defined ({{.InterfaceName | snakecase }}_mock_test.go does provide notification_adapter_mock_test.go, but I want to have adapter_notification_mock_test.go, such that the files are alphabetically ordered close to each other).

We don't currently have a template variable that provides to you the filename of where the interface was defined. The tricky part of this right now is that if you have multiple interfaces in a single file, mockery would not be able to represent that currently because we're restricted to only a single mock per file. That's a feature change I intend to make in a different PR because it will require a fair amount of effort.

Even with inpackage: True and outpkg: "{{.PackageName}}" set, the generated mock does contain an import for the package where the interface is defined, which obviously leads to an import cycle.

Perhaps I don't understand your intention here, but if you want the mock to live physically next to the file, you need to set dir: {{ .InterfaceDir }} if you haven't already. If it's in the same directory then you would not need/want an import for the package because the mock would already be part of the package? Or are you saying it's not generating imports for external interfaces?

On an other example I tried to generate a mock from a type alias: type TelemetryClient = appinsights.TelemetryClient

Yeah these are notoriously tricky. You have to use replace-types to tell mockery to use the alias name, not the underlying name. https://vektra.github.io/mockery/latest/features/#replace-types. This is a limitation of how Go represents type alias in the AST (or rather, how it doesn't represent them).

Edit: it turns out there has been some progress made in go/types to represent a type alias as an explicit node in the type tree: https://github.com/golang/go/issues/44410. Although, their _Alias node is not available for general use it seems. Might be worth revisiting this particular problem later.

Edit 2: very interesting, using the gotypealias flag, we might get access to the alias info: https://pkg.go.dev/golang.org/x/tools/internal/aliases

When GoVersion>=1.22 and GODEBUG=gotypesalias=1, the Type() of the return value is a *types.Alias.

LandonTClipp avatar Feb 14 '24 20:02 LandonTClipp

Even with inpackage: True and outpkg: "{{.PackageName}}" set, the generated mock does contain an import for the package where the interface is defined, which obviously leads to an import cycle.

Perhaps I don't understand your intention here, but if you want the mock to live physically next to the file, you need to set dir: {{ .InterfaceDir }} if you haven't already. If it's in the same directory then you would not need/want an import for the package because the mock would already be part of the package? Or are you saying it's not generating imports for external interfaces?

Yes, I want the mock to live physically next to the file and yes I do have dir: {{ .InterfaceDir }} set. Because I do want it to live in the same directory, I do not want an import for the source package of the interface (since it is the same), but mockery is adding it anyway.

Could it be, that the source of the problem is, that the signature of the interface, that is mocked, accepts a parameter, which is again an interface from the same package?

This is the config I have:

quiet: False
disable-version-string: True
with-expecter: True
style: moq
mockname: "{{.InterfaceName}}Mock"
packages:
  github.com/breml/pkg:
    config:
      dir: "{{.InterfaceDir}}"
      filename: "adapter_notification_mock_test.go"
      style: moq
      outpkg: "{{.PackageName}}"
      inpackage: True
    interfaces:
      NotificationAdapter:

With adapter_notification.go:

package pkg

import "context"

type NotificationAdapter interface {
	Notify(ctx context.Context, itemKey string, item Publication) error
}

and service_publisher.go:

package pkg

type Publication interface {
	ID() string
	Name() string
}

I get adapter_notification_mock_test.go starting as follows:

// Code generated by mockery; DO NOT EDIT.
// github.com/vektra/mockery

package pkg

import (
	"context"
	"sync"

	"github.com/breml/pkg"
)

// Ensure, that NotificationAdapterMock does implement NotificationAdapter.
// If this is not the case, regenerate this file with moq.
var _ NotificationAdapter = &NotificationAdapterMock{}

// NotificationAdapterMock is a mock implementation of NotificationAdapter.
//
//	func TestSomethingThatUsesNotificationAdapter(t *testing.T) {
//
//		// make and configure a mocked NotificationAdapter
//		mockedNotificationAdapter := &NotificationAdapterMock{
//			NotifyFunc: func(ctx context.Context, itemKey string, item pkg.Publication) error {
//				panic("mock out the Notify method")
//			},
//		}
//
//		// use mockedNotificationAdapter in code that requires NotificationAdapter
//		// and then make assertions.
//
//	}
type NotificationAdapterMock struct {
	// NotifyFunc mocks the Notify method.
	NotifyFunc func(ctx context.Context, itemKey string, item pkg.Publication) error

...

Both files (adapter_notification.go and adapter_notification_mock_test.go) live in the same folder, have the same package name but still mockery adds the import for "github.com/some/pkg".

If I remove the item Publication from the Notify method in the NotificationAdapter interface, then the wrong import is no longer added.

breml avatar Feb 14 '24 22:02 breml

I see, that gives me more clarity. I'll need to dig into why that's happening.

Most of the logic being used to generate moq is mostly identical to the original repo so I'll need to figure out why it's different in this case.

LandonTClipp avatar Feb 14 '24 22:02 LandonTClipp

inpackage generation fixed in pkg/fixtures/inpackage/foo_test.go. Fixed a few other bugs as well.

LandonTClipp avatar Feb 21 '24 20:02 LandonTClipp