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

feat: add static validation of generated code for interface #67

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

Conversation

bogdanlytvynovskyi
Copy link

@bogdanlytvynovskyi bogdanlytvynovskyi commented Aug 24, 2023

Requested feature A clear description of the desired feature and an example of
how it would be used.

For each generated interface mock include a line like:
var _ Interface = (*MockInterface)(nil) in mock file

Why the feature is needed A clear description of how this feature is not
served by existing functionality in gomock.

Often build step and generation step are separated. The workflow for developer should look like:

go generate ./...
git add .
git commit
However when working with old interfaces, and updating those - more often than not people tend to forget to do the generation step. If there is no mock usage within this repository everything is going to be fine in terms of build/test for this repository, but consumers of this package in a different repository are going to break. Which requires going back and releasing new "fixed" version. The proposal is to enforce build failure in case generated files are not updated appropriately.

WDYT?

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2023

CLA assistant check
All committers have signed the CLA.

@r-hang r-hang requested review from r-hang and JacobOaks and removed request for JacobOaks August 29, 2023 18:26
@r-hang
Copy link
Contributor

r-hang commented Sep 19, 2023

CI is failing since there is another call to GenerateMockInterface that should be modified. The generated code should also be updated with these new assertions.

If a user updates an externally facing interface, I think external consumers could break with or without mock interface assertions. From my understanding of the issue, I see that this feature would be most helpful for erroring out only situations where there are mocks of interfaces for which the mocks aren't used anywhere in tests.

Do you see this feature as a means to remind developers when they are making interface breaking changes?

@bogdanlytvynovskyi
Copy link
Author

@r-hang

Do you see this feature as a means to remind developers when they are making interface breaking changes?

Yes - this is exactly the point. Unfortunately I see a lot of code where people return interface instead of value.

  • So a repo A has a function returning interface.
  • Repo B has something that was written just to consume that interface.
  • Tests in repo B rely on mocks generated in repo A
  • A breaking change to interface in repo A (mock regen was missed)
  • major version number was not bumped (a lot of times people don't like major version bump due to hassle of removing different versions across different repos)

Unfortunately there is a few bad practices coming together that leads to this situation especially in private repositories.

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