RFC: integration of github.com/matryer/moq-style mocks

[LandonTClipp]

matryer/moq#208 was submitted to the maintainers of matryer/moq suggesting integrating the moq-style mocks into mockery. This suggestion seems well-received due to the benefits we could gain from combining the two projects' strengths.

This GitHub issue serves as a design proposal, discussion, and feature tracking page.

PR Tracker

https://github.com/vektra/mockery/pulls?q=is%3Apr+label%3Amoq+

Background

vektra/mockery style of mocks rely on argument matching behavior provided from testify/mock. Expectations are created by mapping argument values to return values and side effects. Many valid criticisms have been raised about various pitfalls of this kind of expectation system. There are various alternate strategies for defining mock objects, and it's the goal of this project to combine the strengths of these alternate strategies with the strengths implemented in mockery itself.

mockery benefits

Speed

Mockery has undergone a huge rework over the last year with the new packages feature. Historically, mockery (and basically all other mock implementations) relied heavily on using go:generate statements to generate one mock per instantiation of mockery. This was extremely inefficient because it meant the packages.Load call required to parse the AST had to parse the package and all of its dependencies many times. This was an O(n*m) operation (where n is the number of mocks being generated, and m is the number of dependent packages). The rather trivial insight was to simply collect all packages to parse within a single list and pass all of them into packages.Load at once, which removed m from the equation. The new configuration model allows mockery to have this singular list of packages available within a single process.

Configuration inheritance

The other benefit of the configuration model is that allows inheritable configuration. Configuration can be specified at the top-level defaults, at the package level, and at the interface level, with parameters being inherited at each level. This is a powerful way to differentiate mock generation based on individual needs.

Templating

The configuration also takes advantage of Go templating, which allows users to call upon variables that are only known at runtime. Templating functions provide more power and flexibility to users to alter template variables.

mockery criticisms

1. This type of mocking encourages increased coupling between tests due to the fact that it's easy to fall into a habit of asserting how the mock was called, which is bad practice (it asserts implementation details).
2. Despite the great effort in adding type-safe expectation methods (Add mock generation with expecter #396), the necessity of specifying mock.Anything in mock objects means that the expecter's argument types have to be any. This means that argument types are not a compile-time safety guarantee but rather runtime.
3. There is a huge amount of logic in testify that goes into correctly mapping method calls to their corresponding expectations. While testify does its job perfectly well, its clear that the complicated details of argument-matching necessitates a huge amount of logic to support it. A different mocking model could greatly simplify the generated code.

matryer/moq benefits

Simplicity

These style of mocks provide an incredibly simple way of defining mocks: you define the function to be run for your mock call. No argument matching, no assertions on how the mock was called. This simplicity encourages the programmer to not do too much with the mock, which reduces coupling. It doesn't make it impossible to do nasty things like call count and argument value assertions, but it makes it somewhat harder to do.

Type safety

moq objects are strictly type safe, which means test compilation will fail immediately if your mock doesn't satisfy the interface no matter what.

matryer/moq criticisms

Speed

Like all other mock projects besides mockery, moq relies on go:generate to instantiate one moq process per mock object. This is highly inefficient for the reasons aforementioned.

Configuration model

Its configuration model is entirely CLI parameter based. There are no config files, no inheritable config, no use of Go templating.

Proposal

Configuration syntax

moq-style mocks can be easily called upon in mockery by yaml such as this:

with-expecter: true
all: true
packages:
    github.com/my/repo:
        config: 
            style: moq
    github.com/my/other-repo:
        config: 
            style: mockery # this would be the default

Of course, style could be specified anywhere: at the top-level (defaults), in an environment var, in the CLI parameter, at the package level, or at the interface level.

Generated code

To maintain backwards compatibility with all existing usage of moq, the generated code would be identical to that of github.com/matryer/moq, minus any necessary differences to comments within the mock file. For those unfamiliar, this provides an example of the mock moq generates.

Given:

// Person represents a real person.
type Person struct {
	ID      string
	Name    string
	Company string
	Website string
}

// PersonStore provides access to Person objects.
type PersonStore interface {
	Get(ctx context.Context, id string) (*Person, error)
	Create(ctx context.Context, person *Person, confirm bool) error
}

This is generated.

type PersonStoreMock struct {
	// CreateFunc mocks the Create method.
	CreateFunc func(ctx context.Context, person *Person, confirm bool) error

	// GetFunc mocks the Get method.
	GetFunc func(ctx context.Context, id string) (*Person, error)
...

You then define each function individually in your test as such:

func TestSomethingThatUsesPersonStore(t *testing.T) {

    // make and configure a mocked PersonStore
    mockedPersonStore := &PersonStoreMock{
        CreateFunc: func(ctx context.Context, person *Person, confirm bool) error {
            panic("mock out the Create method")
        },
        GetFunc: func(ctx context.Context, id string) (*Person, error) {
            panic("mock out the Get method")
        },
    }

    // use mockedPersonStore in code that requires PersonStore
    // and then make assertions.

}

File placement

Where the mock files are physically placed will be according to the dir parameter, just as in the current system.

Code implementation

Proposal 1 (rejected)

Rejected proposal 1 (expand)

mockery has a concept of an [`Outputter`](https://github.com/vektra/mockery/blob/v2.34.0/pkg/outputter.go#L286), which is the object that determines where a mock should physically reside, and a [`Generator`](https://github.com/vektra/mockery/blob/v2.34.0/pkg/generator.go#L85) which creates the mock code. The `Outputter` instantiates a `Generator` which creates the mock in-memory, and then is flushed to the file handle provided by the `Outputter`.

This relationship will need to change. The Outputter will need to instantiate (or take, as a parameter) a GeneratorV2 (actual name TBD) that parses the interface's AST and provides a set of parameters to some arbitrary Go templating string, the sum of which would provide enough information to generate any arbitrary kind of mock. GeneratorV2 will render the template at the location that Outputter specifies using the template string pointed to (by name or by filepath) in the style parameter.

This methodology of being driven off of templates grants us an enormous amount of flexibility, as the API simply becomes the set of parameters provided to the Go templating engine. Anyone using mockery could then define their own template to create any kind of mock they want.

We will need a backwards compatability layer for mockery style mocks because it is not 100% driven off of templates, so unfortunately GeneratorV2 will only be able to be used on moq style mocks for the initial iteration.

Proposal 2

A new GeneratorTemplate object will be created that does all of the variable-gathering related to the interface in question. The variables gathered will be a similar set that are currently used in mockery's Generator. GeneratorTemplate will be designed in a way that is totally agnostic to the style of mock being generated. Once the variable gathering work has completed, GeneratorTemplate will pass this set of variables to a template string, which will call upon this set of variables. The particular template in use will be controlled by the style parameter.

moq already drives its generation off a single template, shown here. The idea is that we can embed this string in the executable and create a mapping of style names to template string. This scheme will allow mockery to take any arbitrary template string, even user-provided ones, which grants us a large amount of flexibility. The mockery project itself will maintain its own small, curated list of mock templates.

mockery-style mocks unfortunately are only partially driven off of templates, so it will not be able to be ported into the GeneratorTemplate. The code will have to be bifurcated until mockery-style mocks can be completely ported to a pure template.

packages.Load changes

moq currently calls packages.Load based off filesystem path: https://github.com/matryer/moq/blob/c59089b2cd89d975f3d44924f04f37591fbd701d/internal/registry/registry.go#L31

This will need to be modified to integrate with the Interface struct that contains the AST information about each interface discovered in the package. This struct is generated using the information returned from here.

[breml]

Hi @LandonTClipp
Thanks for the proposal and the solid preparation work. I am not yet familiar with the ins and outs of mockery, so I am not yet able to comment on all the aspects of the proposal. But still let me share some thoughts:

It doesn't make it impossible to do nasty things like call count and argument value assertions, but it makes it somewhat harder to do.

As a experienced user of moq, I do not agree with this observation. Count assertions are extremely simple, e.g. len(mockedPersonStore.CreateCalls()) for the mock in the proposal. For argument assertions, it is my observation, that the impact of the arguments on the test result is in most cases less important than the result, that is returned from the mock. That being said, if a mock is just called once (per test execution) again, it is straight forward to have the necessary assertions in the function body of the mocked function. The big advantage in this case of moq is, that the assertions look the same as all the other assertions (in contrast to the clumsy for, that is required by testify with EXPECT().
Maybe the most "challenging" thing for most users that work with moq generated mocks are the proper handling of multiple calls to the same mock in a single test case. For this, the approach with queues served us well (see matryer/moq#187 (comment), unfortunately, this tip did not yet make it into the README.md).

The other benefit of the configuration model is that allows inheritable configuration. Configuration can be specified at the top-level defaults, at the package level, and at the interface level, which parameters being inherited at each level. This is a powerful way to differentiate mock generation based on individual needs.

and

Its configuration model is entirely CLI parameter based. There are no config files, no inheritable config, no use of Go templating.

I think this critic is debatable as well. As long as there is no need for just an other config file, that pollutes my repository, since everything that is needed to generate the mocks fits into the go:generate line is a good thing in my opinion. It gets even worse in my opinion, if the config files are spread through out the repository and are then combined in some sort of Configuration inheritance, since this can hurt maintainability. In regards to inheritance in config files, it might be worth to have a look at cue and the reasoning behind, why cue does not have any sort of inheritance (source, e.g. core developer Marcel van Lohuizen in "Override-style inheritance is the biggest source of complexity in configuration." in https://www.coss.community/cossc/ocs-2020-breakout-marcel-van-lohuizen-google-17i0).

style

In regards to the possible values for the configuration attribute style, I wonder if it does make sense to have the "product" names there or if we should try to find terms, that describe best the two different styles of mocks.

Generator

I wonder, if the generator could be made "generic" in the sense that it just uses Go templates again to generate the final result. Similar to how it is done in https://github.com/hexdigest/gowrap. Basically mockery would just collect the necessary information (state), that is needed to generate the mocks and pass this to the Generator, which then basically loads a Go template where the final result is generated based on the state that is passed.

I hope, this thoughts are helpful for you.

[breml]

I myself am not super familiar with moq so it's great to have you in this discussion :D

❤️

I was a bit confused as to why this would be a "challenge," but what you suggested (either an external counter or a queue of return values) is what I thought would be done. In fact, the reason why I like that method more is precisely because it's ugly/manual. I've found it to generally be (like 80% of the time) bad practice, so I feel the mock framework should discourage it by default due to the increased coupling it creates. The main criticisms that have been raised is that mockery encourages this sort of coupling due to how easy it is to say .EXPECT().Foo(1).Return(1).Once().

I fully agree, the counting as well as the queue thing are pretty rarely used with moq in my experience, but there are well justified cases.

I agree, on the surface this is a matter of taste. I prefer centralized config files, some people prefer go:generate. In this case, our hand is forced because go:generate was in fact the sole reason for poor performance. So in mockery, we'll never go back to recommending go:generate. It's a trade off that I feel is worth it for the 5x increase in performance I've measured.

I completely understand the reasoning for the config file and also here, I agree, that a 5x times increase in performance does justify an other config file. I was more challenging the absoluteness of the statement, that "... configuration model is entirely CLI parameter based ..." is a valid critic (on its own). In combination with the performance increase, this is a completely different story.

This had been suggested, but I rejected the idea on the grounds that it didn't seem like a highly requested feature and there are ways around the lack of inheritable config files. So as it stands, the only inheritance is your typical 3 sources of environment, CLI, and config files (and of course the mappings within the config file itself).

I think I do not yet understand the form of inheritance that is possible in the configuration of mockery. The 3 ways of providing config values (env, CLI, config files) is not what I consider inheritance (and I don't think this layering is a problem).

You are probably right. Maybe: expectation and function-attribute? Open to other ideas.

Or just expect and func, where expect referenced the EXPECT "constructor" and func references the Func suffix, of the attributes of the mock that are needed to be populated.

That's a great idea. We could probably structure a new kind of generator that does just that. Unfortunately it would be very hard to port over the current Generator that creates expectation mocks because it has a lot of in-memory Sprintf-s that construct various parts of the mock. I've been pushing aggressively to move over to a singular template string but that work isn't completed yet. Regardless, we should 100% design it in this way. The other really cool thing about this idea is that we could allow users to define their own templates to create their own style of mocks. At that point, mockery becomes this sort of meta-mocking thing and opens up the door for lots of interesting paths.

Exactly my thought. If I recall some of the feature requests that have been discussed for moq in the past, quite some of them would have been easily resolved by the user them selves by just adjusting their template to their needs (e.g. return zero values for not defined functions instead of panic, provide some sort of mock reset functionality).

[breml]

In regards to:

The other really cool thing about this idea is that we could allow users to define their own templates to create their own style of mocks. At that point, mockery becomes this sort of meta-mocking thing and opens up the door for lots of interesting paths.

we might even think bigger, where mockery can not only be used to generate configuration based mocks but also middlewares (as done with the before mentioned https://github.com/hexdigest/gowrap. We use gowrap at work to generate all sorts of middlewares (logging, metrics, etc.) in a medium sized mono-repo and this already now is pretty slow (since it relies on go:generate). Therefore I guess, with the Go template based approach, it could become possible to also replace gowrap with mockery and profit from the performance boost, that the configuration based approach provides.

[breml]

@LandonTClipp looks like you are on the Thought Works Tech Radar, congrats.

[sudo-suhas]

In regards to the possible values for the configuration attribute style, I wonder if it does make sense to have the "product" names there or if we should try to find terms, that describe best the two different styles of mocks.

I think it is really difficult to find the right names that appropriately convey the type of mock that would be generated based on the end-user's setting. Considering that, it might be better to use the "product" names as the style names since that would clearly convey the type of mock to expect.

[LandonTClipp]

@breml sorry for the late reply here, yes all great thoughts. I think we're in agreement that doing a template-based approach is going to give us the most lift. I'll update the proposal with that suggestion, thanks!

I have never heard of Thought Works Tech Radar, but that's great nonetheless!

[sudo-suhas]

packages.Load changes

moq currently calls packages.Load based off filesystem path: https://github.com/matryer/moq/blob/c59089b2cd89d975f3d44924f04f37591fbd701d/internal/registry/registry.go#L31

This will need to be modified to integrate with the Interface struct that contains the AST information about each interface discovered in the package. This struct is generated using the information returned from here.

One concern I have with this is that we would be requiring circular dependency between the two packages. Mockery would need to import the implementation of CodeGenerator from Moq while Moq would need to import the Mockery package integrating with the Interface struct.

[LandonTClipp]

I think the idea is more that the moq code will live in this repo. So I don't think there will be any circular dependency issues. Even if there were, that's easily resolvable with a little bit of restructuring IMO.

If you are okay with that I would love to bring you and anyone else intimately familiar with the maintenance of moq as collaborators into this repo.