Skip to content

Switch mock generation from **moq** to **mockery** (build-tag test) markdown #877

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Switch mock generation from **moq** to **mockery** (build-tag test) markdown #877

wants to merge 2 commits into from

Conversation

reject-i
Copy link

@reject-i reject-i commented May 2, 2025 •
edited
Loading

What’s inside

  • .mockery.yaml – central config (mock-build-tags: "test", inpackage, with-expecter).
  • Replaced every //go:generate moq … with //go:generate mockery --name=….
  • Deleted legacy *_generated_mock.go / *_moq_test.go; added new mock_*.go (guarded by //go:build test).
  • Makefile targets mocks / generate_mocks
    • regenerate SSZ first
    • iterate over packages that contain //go:generate mockery and run go generate.
  • CI / local build: mocks stay out of production binary, are reproducible via make mocks.

Closes #852

@reject-i reject-i mentioned this pull request May 2, 2025
Closed
4 tasks
@reject-i reject-i changed the title Switch mock generation from **moq** to **mockery** (build-tag test) markdown Копіювати Редагувати Switch mock generation from **moq** to **mockery** (build-tag test) markdown May 2, 2025
@ukorvl ukorvl requested a review from Un1oR May 2, 2025 19:06
olegrok
olegrok requested changes May 5, 2025
View reviewed changes
@reject-i reject-i force-pushed the mockery-migration branch from d65e913 to 180c964 Compare May 5, 2025 14:04
@reject-i
Copy link
Author

reject-i commented May 5, 2025

  • switched mock generation from matryer/moq to mockery
  • added .mockery.yaml with build tag test, inpackage, with-expecter, package list & include-regex
  • removed //go:generate directives – generation is now driven solely by make mocks
  • ignored nil/**/mock_*.go and dropped committed auto-generated mocks
  • simplified mocks targets in Makefile & nil/Makefile.inc

@Un1oR @olegrok ready for review , thanks

olegrok
olegrok reviewed May 5, 2025
View reviewed changes
nil/services/relayer/Makefile.inc

.PHONY: $(root_relayer)/gen_l2_mocks
$(root_relayer)/gen_l2_mocks: $(root_relayer)/embed_l2_abi
cd $(root_relayer)/internal/l2 && go run github.com/matryer/moq -out l2_contract_generated_mock.go -rm -stub -with-resets . L2Contract
cd $(root_relayer)/internal/l2 && mockery --name=L2Contract
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try to build a project?

.gitignore
@@ -85,3 +85,8 @@ openrpc.json
.pnpm/
.pnpm-store/
.qodo
nil/**/mock_*.go

# mockery output (исключаем helper-тесты)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we write comments in English even we use AI tools

zadykian
zadykian reviewed May 5, 2025
View reviewed changes
.gitignore
@@ -85,3 +85,8 @@ openrpc.json
.pnpm/
.pnpm-store/
.qodo
nil/**/mock_*.go

# mockery output (исключаем helper-тесты)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write all comments in English

.mockery.yaml
nil/internal/vm: {}
nil/services/rollup: {}

include-regex: '^(Client|DB|RwTx|StateDBReadOnly|L1BlockFetcher)$'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still plenty of types used for mock generation which are not covered here
For instance, check this Makefile.inc

.mockery.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I see, mockery has a completely different API for assertions in comparison to moq
So, all existing tests should also be adjusted

@zadykian
Copy link
Member

zadykian commented May 5, 2025 •
edited
Loading

Do we really need to migrate to mockery?
What benefits are we getting by adding a new build dependency and updating all existing tests?

Regarding the //go:build test annotation, it can be easily handled by a script

Script invocation looks like that:

//go:generate bash ../scripts/generate_mock.sh {InterfaceName}

@Un1oR
Copy link
Contributor

Un1oR commented May 6, 2025

@zadykian I'll answer as the author of the original proposal)

As far as I see, mockery is more actively maintained and developed than moq. Maybe it's not so important for a "just working" library, but still.
OK, we could add workaround for build tags, but why when it's already there out of the box?
I like the idea of being able to remove unnecessary stuff from Makefiles. The boilerplate for various code generation in them makes me sad.
Also mockery positions itself not only as a mock generator, but also as a framework for generating general purpose code using customizable templates. This could be interesting for replacing reflection with codegeneration in RawAPI, for example.
Well, and adding one dependency instead of another doesn't look like a big problem.
And test adaptation is part of the task. If it is accomplished, we don't have to worry about additional effort.

@reject-i
Copy link
Author

reject-i commented May 6, 2025

Should I apply all your suggestions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olegrok olegrok olegrok requested changes

@zadykian zadykian zadykian left review comments

@Un1oR Un1oR Awaiting requested review from Un1oR

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@reject-i @zadykian @Un1oR @olegrok