Skip to content

Support gomock#1030

Open
kszafran wants to merge 7 commits intovektra:v3from
kszafran:support-gomock
Open

Support gomock#1030
kszafran wants to merge 7 commits intovektra:v3from
kszafran:support-gomock

Conversation

@kszafran
Copy link
Copy Markdown

@kszafran kszafran commented Apr 26, 2025

Description

This PR adds support for generating gomock mocks. It's a follow-up to a PR I created directly to gomock, trying to add a batch mode in order to reduce generation time.

Using mockery, I got an even better improvement in generation time. Calling mockgen on 78 packages in my project sequentially:

       61.13 real       270.97 user       132.04 sys

Running mockery to generate them all at once:

        2.23 real         2.76 user         7.13 sys

which is almost 30x reduction in real time (40x in CPU time).

This PR also includes a few small improvements:

  • calling SignatureNoName() will now avoid wrapping single return arg in parentheses — (string) -> string
  • types are now not repeated in arg lists and return arg lists if not necessary — (a int, b int, c int) -> (a, b, c int)
  • a few small features added to support the new template: .SrcPkgName, .SrcPkgPath, .Interfaces.NameList, list and append functions

I tried to keep the template file close in structure to the original mockgen code, so that it's easy to update in the future to reflect changes in mockgen.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Version of Go used when building/testing:

  • 1.22
  • 1.23
  • 1.24

How Has This Been Tested?

I regenerated mocks using mockery in my private project. The output is pretty faithful to the code generated by mockgen.
The differences I noticed are:

  • Different file header. I tried to keep the style close to the existing mockery templates. Generated by mockgen:
    // Code generated by MockGen. DO NOT EDIT.
    // Source: github.com/owner/repo/path/to/pkg (interfaces: Interface,Another)
    //
    // Generated by this command:
    //
    //	mockgen -typed -destination=mockpkg/mocks.gen.go -package=mockpkg . Interface,Another
    //
    
    Generated by mockery:
    // Code generated by mockery; DO NOT EDIT.
    // github.com/vektra/mockery
    // template: gomock
    // source: github.com/owner/repo/path/to/pkg (interfaces: Interface, Another)
    
  • Imports aren't aliased if not necessary (mockgen always uses aliases like gomock "go.uber.org/mock/gomock").
  • mockery always suggests some nice arg names in Do, Return, DoAndReturn (these functions take the return args of the mocked method), while mockgen uses arg0, arg1, ...
  • mockery keeps arg names in function types (f func(projectIDs []string)), while mockgen drops them (f func([]string))

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@LandonTClipp
Copy link
Copy Markdown
Member

Wow! That's wonderful! I will take a closer look next week. In the meantime, it's worth opening a dialogue with the gomock maintainers because the question of whether we host the template in this project or not is really a question on whether gomock itself should be forked (as that is effectively what it would mean to host it here).

@kszafran
Copy link
Copy Markdown
Author

I couldn't help myself and made two further improvements (two new commits):

  • generate mocks in parallel — this further brought down the generation time to ~1.7-2s
  • avoid reparsing templates and recompiling schemas (remote schemas were already compiled just once, but not the built-in ones, and templates were always parsed for every package) — didn't see any reliable effect on performance, but it can't hurt

In the meantime, it's worth opening a dialogue with the gomock maintainers [...]

I left a comment in my original issue (uber-go/mock#245). To be precise, this would be forking only mockgen (the code generation part), not the whole gomock library. There are some small details that I didn't port to the mockery template (like optionally adding a //go:generate mockgen ... comment, or the ability to ingest a gob-encoded package to avoid reparsing code).

@n-oden
Copy link
Copy Markdown

n-oden commented Aug 24, 2025

Has there been any further movement on getting this merged / coordinating with the gomock team? An order-of-magnitude improvement in mockgen time is something I'd very much like to see, and I suspect I'm far from alone!

@LandonTClipp
Copy link
Copy Markdown
Member

Has there been any further movement on getting this merged / coordinating with the gomock team? An order-of-magnitude improvement in mockgen time is something I'd very much like to see, and I suspect I'm far from alone!

@n-oden no they seem unresponsive. The likelihood they will support something like this is very very low. I'd be open to the idea of merging this as an experiment but without explicit support on their side, I cannot guarantee the correctness of it.

@n-oden
Copy link
Copy Markdown

n-oden commented Feb 13, 2026

@LandonTClipp that's aggravating. For what it's worth, I'd support merging this with some sort of experimental tag/designation: I (and I suspect many others) don't need ongoing compatibility with go-mock, I just need an off-ramp that lets me convert to something that won't waste hours of cpu time per day in my CI pipeline.

@LandonTClipp
Copy link
Copy Markdown
Member

Doing it as an experiment is totally fine. Give me a few days to revisit this and maybe we can get it merged. Ping me if I don't respond in a week.

@LandonTClipp
Copy link
Copy Markdown
Member

After looking at this PR again, I want to merge the gomock bits but there are many irrelevant changes that need to be taken out before we can do this. For example:

  1. Changes in variable naming as seen in e2e/test_template_exercise/exercise_expected.txt.
  2. Refactoring of various data structures: https://github.com/vektra/mockery/pull/1030/changes#diff-b646654cbc705595cc00438ec5afb23b611f39ab1b55cfa0bb5e8de1b9ac543fR58-R81 and https://github.com/vektra/mockery/pull/1030/changes#diff-dd1c701f26e4b66f2c1c1b312fd78581f706770353861da4095b1fbf740c47deR58-R103 and https://github.com/vektra/mockery/pull/1030/changes#diff-dd1c701f26e4b66f2c1c1b312fd78581f706770353861da4095b1fbf740c47deR449

Touching such a large number of pieces of the project in one PR is dangerous. I'm not opposed in principle to the changes, just not all in one PR.

@kszafran what do you think? Can you reduce the changes being proposed in this PR to just what is relevant to make gomock work?

@kszafran
Copy link
Copy Markdown
Author

After looking at this PR again, I want to merge the gomock bits but there are many irrelevant changes that need to be taken out before we can do this. For example:

  1. Changes in variable naming as seen in e2e/test_template_exercise/exercise_expected.txt.
  2. Refactoring of various data structures: https://github.com/vektra/mockery/pull/1030/changes#diff-b646654cbc705595cc00438ec5afb23b611f39ab1b55cfa0bb5e8de1b9ac543fR58-R81 and https://github.com/vektra/mockery/pull/1030/changes#diff-dd1c701f26e4b66f2c1c1b312fd78581f706770353861da4095b1fbf740c47deR58-R103 and https://github.com/vektra/mockery/pull/1030/changes#diff-dd1c701f26e4b66f2c1c1b312fd78581f706770353861da4095b1fbf740c47deR449

Touching such a large number of pieces of the project in one PR is dangerous. I'm not opposed in principle to the changes, just not all in one PR.

@kszafran what do you think? Can you reduce the changes being proposed in this PR to just what is relevant to make gomock work?

Commits in this PR are mostly self-contained, so we could e.g. pull a few initial commits into a separate PR. That would make sense if you squash the commits on merge. But it seems to me that this isn't the case in this repository. Since individual commits would be preserved after merge, I'm not sure what the benefit would be? I can try to split off some commits anyway.

@LandonTClipp
Copy link
Copy Markdown
Member

It's more for reviewers like me, not git history. It's harder to give attention to so many changes in one PR.

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

Successfully merging this pull request may close these issues.

3 participants