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

Autogenerate mockery mocks #2215

Merged
merged 19 commits into from Jun 5, 2019

Conversation

3 participants
@mkrump
Copy link
Contributor

commented May 31, 2019

Description

Allow for the autogeneration of mockery mocks using go generate

Reviewer Notes

  • mockery takes an -all parameter, which would generate mocks for all our interfaces, but this was pretty slow, so instead opted for adding a //go:generate ... comment only for the interfaces that we want to generate mocks for.
  • Double check the Makefile arrangement. Maybe shouldn't be calling from server_test, but seems nice to autogenerate them when running the tests. It could slow things down if we have a lot of mocks.
  • I tried to avoid having to supply the path -output=$GOPATH/src/github.com/transcom/mymove/mocks to go generate, but without it the mocks are created in the same directory as the interface, rather than a single mock directory like our current setup. I wasn't sure if we were willing to change the layout or if the more verbose go generate command was the lesser of two evils. Also it's not too bad, since you can add a template to your IDE (in GoLand for example: $GOPATH/src/github.com/transcom/mymove/bin/mockery -name $NAME$ -output=$GOPATH/src/github.com/transcom/mymove/mocks).
  • As an aside, one benefit that I could see from putting the mocks in the same dir as the pkg they are mocking over putting all of our mocks a in single top level mocks directory, is that eventually we'll have two interfaces with the same name and we'll have a name clash in the top level mocks dir. Also, having the mocks in the pkg with the interface that they are mocking, makes it easier to find the interface that we're mocking imo.

Setup

  1. delete the mocks directory.
  2. run make generate_mocks and the mocks should be back.

mkrump added some commits May 31, 2019

@mkrump mkrump added the roci label May 31, 2019

@mkrump mkrump requested a review from rdhariwal Jun 3, 2019

Show resolved Hide resolved docs/how-to/generate-mocks-with-mockery.md Outdated
Show resolved Hide resolved pkg/services/accesscode.go Outdated
@@ -401,28 +406,32 @@ else
endif
endif

.PHONY: generate_mocks
generate_mocks: bin/mockery
go generate $$(go list ./... | grep -v \\/pkg\\/gen\\/ | grep -v \\/cmd\\/)

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 3, 2019

Contributor

If this is slow, at some point, we can get fancy and only update changed files since the last run. I think this is ok for now though.

Makefile Outdated
.PHONY: server_test
server_test: server_deps server_generate db_test_reset db_test_migrate ## Run server unit tests
server_test: server_deps server_generate generate_mocks db_test_reset db_test_migrate ## Run server unit tests

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jun 3, 2019

Contributor

Another option is to remove it from server_test so that CI doesn't have to deal with it. Then just have people run it locally. The run time becomes less of an issue.

This comment has been minimized.

Copy link
@mkrump

mkrump Jun 3, 2019

Author Contributor

I think i kind of prefer your suggestion. which way do you lean?

mkrump added some commits Jun 3, 2019

PR Feedback:
* Move mocks dir to directory where defined
* Update instructions to only use go generate for mock generation
* Remove mock generation from tests
Show resolved Hide resolved Makefile Outdated
@chrisgilmerproj
Copy link
Contributor

left a comment

I don't like that the generated mock code get's merged into master. I'd prefer that it be removed and generated each time to ensure we have clean and accurate builds in our CI system. I'd like it to be similar to the Swagger generated code or the the assets code which is .gitignore'd and generated by CI when it's needed for testing and/or build.

So I'd like to see something added to make clean that removes this code and an addition to .gitignore and .dockerignore. I think we also want to add the tool itself to pkg/tools/tools.go.

I'm 💯 for this new automation so thanks for doing this.

@mikena-truss

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@chrisgilmerproj I think the issue was how slow it is. Swagger generation is seconds, if we end up using the mocks across the codebase, it could be more like a couple minutes. I agree it's cleaner, but wondering if we want to add that much time to the build?

@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

@chrisgilmerproj I think the issue was how slow it is. Swagger generation is seconds, if we end up using the mocks across the codebase, it could be more like a couple minutes. I agree it's cleaner, but wondering if we want to add that much time to the build?

If time is an issue we can certainly make this generation of code its own job in CircleCI. And that would mean we could cache it for other build steps. My concern is checking in code that may not be correct that doesn't get fix in a re-generation step which in turn might offer a way to have bad code leak into our application. I'd much rather just have it generated by CircleCI instead of checked in by us. I'm happy to help with the CircleCI stuff for it too.

PR Feedback
* Add mocks to .gitignore .dockerignore
* Add clean command for mocks
* Add mockery to tools.go and
* Add target for /bin/mockery and
* Revert pathing in go generate to use full mockery path
@codecov

This comment has been minimized.

Copy link

commented Jun 3, 2019

Codecov Report

Merging #2215 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #2215      +/-   ##
=========================================
- Coverage   59.01%     59%   -0.01%     
=========================================
  Files         250     250              
  Lines       14071   14025      -46     
=========================================
- Hits         8303    8275      -28     
+ Misses       4767    4751      -16     
+ Partials     1001     999       -2

mkrump and others added some commits Jun 3, 2019

Show resolved Hide resolved Makefile
@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@mikena-truss @chrisgilmerproj apologies for the churn. took a bit of experimentation to figure out the pathing on circleci. included both of your feedback. let me know if anything else.

Makefile Outdated
@@ -401,28 +406,32 @@ else
endif
endif

.PHONY: generate_mocks
generate_mocks: bin/mockery

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 4, 2019

Contributor

nit: I'd really like this to be mocks_generate so it reads the same as server_generate.

This comment has been minimized.

Copy link
@mkrump

mkrump Jun 4, 2019

Author Contributor

np. updated.

Makefile Outdated
@@ -401,28 +406,32 @@ else
endif
endif

.PHONY: generate_mocks
generate_mocks: bin/mockery

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Jun 4, 2019

Contributor

I also want you to consider adding a .mocks_generate.stamp file. Then this would only get run once unless someone clears the stamp, which is essentially how we do the server generation.

This comment has been minimized.

Copy link
@mkrump

mkrump Jun 4, 2019

Author Contributor

makes sense. added.

@chrisgilmerproj
Copy link
Contributor

left a comment

Just a couple of changes I've requested in the comments.

mkrump added some commits Jun 4, 2019

PR Feedback:
* Change generate_mocks name to mocks_generate
* Add .mocks_generate.stamp
@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 - Thanks for improving our automation!

@mkrump

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

@mikena-truss let me know if changes look ok. feel free to ping me on slack if i missed anything.

@mikena-truss
Copy link
Contributor

left a comment

LGTM. Thanks for doing this!

@mkrump mkrump merged commit 13acb09 into master Jun 5, 2019

18 of 19 checks passed

codecov/project/go 58.82% (-0.01%) compared to b2afb0d
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing b2afb0d...927e553
Details

@mkrump mkrump deleted the autogen-mocks branch Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.