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

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

Open
wants to merge 54 commits into
base: master
Choose a base branch
from

Conversation

LandonTClipp
Copy link
Collaborator

@LandonTClipp LandonTClipp commented Oct 19, 2023

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

  • Implement all variables needed by moq.templ
  • Create mocks for a variety of fixtures
  • Create tests for the generated moq implementations
  • 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 LandonTClipp changed the title Moq Add github.com/matryer/moq style mocks into mockery Oct 19, 2023
@LandonTClipp
Copy link
Collaborator Author

LandonTClipp commented Nov 21, 2023

@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.

@breml
Copy link

breml commented Nov 21, 2023

@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:

@LandonTClipp
Copy link
Collaborator Author

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.

sudo-suhas and others added 19 commits December 22, 2023 19:19
* Internal registry for disambiguated imports, vars
- Move functionality in the moq package partially into
  internal/{registry,template}.
- Leverage registry to assign unique package and variable/method
  parameter names. Use import aliases if present in interface source
  package.
BREAKING CHANGE: When the interface definition does not mention the
parameter names, the field names in call info anonymous struct will be
different.
The new field names are generated using the type info (string -> s,
int -> n, chan int -> intCh, []MyType -> myTypes, map[string]int ->
stringToInt etc.).
For example, for a string parameter previously if the field name was
'In1', the new field could be 'S' or 'S1' (depends on number of
string method parameters).
* Refactor golden file tests to be table-driven
* Fix sync pkg alias handling for moq generation
* Improve, add tests (increase coverage)
* Use $.Foo in template, avoid declaring variables
$ is set to the data argument passed to Execute, that is, to the
starting value of dot.
Variables were declared to be able to refer to the parent context.
* Consistent template field formatting
* Use tabs in generated Godoc comments' example code
* Minor simplification
* go generate
* Fix conflict for generated param name of pointer type

Excellent work by @sudo-suhas.
Allow the generation of mocks for generics as introduced in golang 1.18
The optional with-resets flag adds methods to reset method calls. Calls
can be reset individually or all at once with these.
* Internal registry for disambiguated imports, vars
- Move functionality in the moq package partially into
  internal/{registry,template}.
- Leverage registry to assign unique package and variable/method
  parameter names. Use import aliases if present in interface source
  package.
BREAKING CHANGE: When the interface definition does not mention the
parameter names, the field names in call info anonymous struct will be
different.
The new field names are generated using the type info (string -> s,
int -> n, chan int -> intCh, []MyType -> myTypes, map[string]int ->
stringToInt etc.).
For example, for a string parameter previously if the field name was
'In1', the new field could be 'S' or 'S1' (depends on number of
string method parameters).
* Refactor golden file tests to be table-driven
* Fix sync pkg alias handling for moq generation
* Improve, add tests (increase coverage)
* Use $.Foo in template, avoid declaring variables
$ is set to the data argument passed to Execute, that is, to the
starting value of dot.
Variables were declared to be able to refer to the parent context.
* Consistent template field formatting
* Use tabs in generated Godoc comments' example code
* Minor simplification
* go generate
* Fix conflict for generated param name of pointer type

Excellent work by @sudo-suhas.
When the type and the package name is the same for an anonymous
parameter (ex: time.Time), and there are more than 1 such parameters,
the generated name for both was the same. And the generated code would
not be valid.

Fix the bug by ensuring the parameter name does not conflict with
package imports first before checking against other parameter names.
When a custom type has a name that can shadow an in-built type, append
MoqParam to the name. Example: var name stringMoqParam for null.String.
Allow the generation of mocks for generics as introduced in golang 1.18
When we generate a unique name for pkg a on conflict with pkg b, the new
name could already be in use for pkg c. So recursively check each unique
name against existing imports before setting it.
…methods

In this commit, we gather all the template data needed by the moq logic to
generate its template. This is untested as of yet.

TODO: need to start testing this works by calling upon `moq` in `.mockery.yaml`.
@LandonTClipp LandonTClipp marked this pull request as ready for review February 13, 2024 20:13
Copy link

codecov bot commented Feb 13, 2024

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 ⚠️
...com/vektra/mockery/v2/pkg/fixtures/expecter_moq.go 0.00000% 145 Missing ⚠️
.../mockery/v2/pkg/fixtures/requester_variadic_moq.go 0.00000% 118 Missing ⚠️
pkg/registry/registry.go 0.00000% 113 Missing ⚠️
pkg/registry/method_scope.go 0.00000% 85 Missing ⚠️
...ub.com/vektra/mockery/v2/pkg/fixtures/fooer_moq.go 0.00000% 84 Missing ⚠️
...ery/v2/pkg/fixtures/imports_same_as_package_moq.go 0.00000% 76 Missing ⚠️
...ktra/mockery/v2/pkg/fixtures/async_producer_moq.go 0.00000% 72 Missing ⚠️
pkg/registry/var.go 0.00000% 70 Missing ⚠️
...ery/v2/pkg/fixtures/requester_return_elided_moq.go 0.00000% 56 Missing ⚠️
... 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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LandonTClipp
Copy link
Collaborator Author

@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.

@breml
Copy link

breml commented Feb 14, 2024

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.

@LandonTClipp
Copy link
Collaborator Author

LandonTClipp commented Feb 14, 2024

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: golang/go#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.

@breml
Copy link

breml commented Feb 14, 2024

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.

@LandonTClipp
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

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

Successfully merging this pull request may close these issues.