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

build: use go generate for mocks #998

Merged
merged 7 commits into from
Apr 11, 2022
Merged

build: use go generate for mocks #998

merged 7 commits into from
Apr 11, 2022

Conversation

ryanchristo
Copy link
Member

@ryanchristo ryanchristo commented Apr 8, 2022

Description

Closes: #817

Use go generate for generating mock code and add data module. Also removes unused mocks.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@ryanchristo ryanchristo mentioned this pull request Apr 8, 2022
19 tasks
@codecov
Copy link

codecov bot commented Apr 8, 2022

Codecov Report

Merging #998 (272038e) into master (43fe06e) will decrease coverage by 0.07%.
The diff coverage is n/a.

❗ Current head 272038e differs from pull request most recent head 19e4b20. Consider uploading reports for the commit 19e4b20 to get more accurate results

@@            Coverage Diff             @@
##           master     #998      +/-   ##
==========================================
- Coverage   72.33%   72.26%   -0.08%     
==========================================
  Files         209      212       +3     
  Lines       23448    23508      +60     
==========================================
+ Hits        16961    16987      +26     
- Misses       5138     5169      +31     
- Partials     1349     1352       +3     
Flag Coverage Δ
experimental-codecov 72.15% <ø> (-0.19%) ⬇️
stable-codecov 67.05% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

tACK

Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

So how is this used? Do we need some docs?

@ryanchristo
Copy link
Member Author

ryanchristo commented Apr 8, 2022

So how is this used? Do we need some docs?

You just need to run go generate instead of make regen-mocks.

We didn't have any prior documentation to update for make regen-mocks. Where would be the best place to add something for go generate? It looks like contributing could use an overhaul.

@ryanchristo
Copy link
Member Author

Opened #1003

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

let's move the directives next to the related types and add go generate ./... to Makefile.

generate.go Outdated
//go:generate mockgen -source=x/ecocredit/expected_keepers.go -package mocks -destination x/ecocredit/mocks/expected_keepers.go
//go:generate mockgen -source=x/ecocredit/expected_keepers.go -package mocks -destination x/ecocredit/mocks/expected_keepers.go
//go:generate mockgen -source=x/ecocredit/server/basket/keeper.go -package mocks -destination x/ecocredit/server/basket/mocks/keeper.go
//go:generate mockgen -source=x/ecocredit/expected_keepers.go -package mocks -destination x/ecocredit/mocks/expected_keepers.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

normally we put the directive next to the related types. So the line above should be placed in x/ecocredit/expected_keepers.go file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do. I found this as an example but looks like best practice is on a per file basis.

Copy link
Member Author

@ryanchristo ryanchristo Apr 8, 2022

Choose a reason for hiding this comment

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

I see why this was an example. You have to run go generate ./... from within each directory and this was being used as a convenient way to generate all mocks with one command.

@ryanchristo ryanchristo changed the title chore: use go generate instead of make build: use go generate instead of make Apr 8, 2022
@robert-zaremba
Copy link
Collaborator

@aaronc , the advantage over make is:

  1. go tools will be part of the go module (as a versioned dependency) - so we can control which version we are using
  2. we put the related directives next to the related types (interfaces)

@ryanchristo
Copy link
Member Author

ryanchristo commented Apr 8, 2022

let's move the directives next to the related types and add go generate ./... to Makefile.

Where should go generate be added to the Makefile? I thought the idea was to avoid using make? The issue is titled, "Use go generate instead of make for mocks". Maybe I misunderstood?

@ryanchristo
Copy link
Member Author

ryanchristo commented Apr 8, 2022

So go generate ./... does not work from the root directory when placed within each file. You have to run:

go generate x/data/expected_keepers.go
go generate x/ecocredit/expected_keepers.go
go generate x/ecocredit/server/basket/keeper.go
go generate x/ecocredit/server/core/keeper.go

Or you have to navigate to each directory and run go generate ./...

@ryanchristo
Copy link
Member Author

Would be happy to make changes but not sure how to best proceed? @robert-zaremba

@technicallyty
Copy link
Contributor

let's move the directives next to the related types and add go generate ./... to Makefile.

Where should go generate be added to the Makefile? I thought the idea was to avoid using make? The issue is titled, "Use go generate instead of make for mocks". Maybe I misunderstood?

the issue was probably go generate vs mockgen, so i think adding the go generate lines to the makefile is good 👍🏻

@robert-zaremba
Copy link
Collaborator

the issue was probably go generate vs mockgen

The issue is with multiple modules. ./... will only take current module into the account. We have the same problem with tests. In fact it's not guaranteed that every tools will work with a root module when there are multiple submodules.

Where should go generate be added to the Makefile?
Would be happy to make changes but not sure how to best proceed?

Simple generate: job would be perfect. I think the following will work:

generate: 
    find -name go.mod -execdir go generate ./... \;

Makefile Outdated
Comment on lines 413 to 414
generate:
find . -name 'go.mod' -type f -execdir sh -c 'go generate ./...' \;
Copy link
Member Author

Choose a reason for hiding this comment

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

This will only work for go generate commands within the same directory as go.mod. We had mockgen set up for server/core and server/basket but those were empty and unused.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to wrap the go generate command in sh

@ryanchristo ryanchristo changed the title build: use go generate instead of make build: use go generate for mocks Apr 11, 2022
Makefile Outdated
Comment on lines 413 to 414
generate:
find . -name 'go.mod' -type f -execdir sh -c 'go generate ./...' \;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you need to wrap the go generate command in sh

@ryanchristo ryanchristo enabled auto-merge (squash) April 11, 2022 23:27
@ryanchristo ryanchristo merged commit f70453c into master Apr 11, 2022
@ryanchristo ryanchristo deleted the ryan/817-go-generate branch April 11, 2022 23:31
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.

Use go generate instead of make for mocks
4 participants