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

Fix infinity mocking caused interface in mock #472

Merged
merged 1 commit into from Jun 14, 2022

Conversation

grongor
Copy link
Contributor

@grongor grongor commented Jun 4, 2022

Description

When you fixed this issue #461 you introduced an annoying bug. πŸ˜„ You can't run mockery multiple times as it will generate mocks of the mocks themselves because each mock now contains an interface (which is itself a candidate for mock generation).

Two examples; first with --keep-tree (infinite mocks) and second without (error on third run):

$ ~/tmp$ tree
.
β”œβ”€β”€ go.mod
β”œβ”€β”€ go.sum
└── lorem
    └── interface.go

1 directory, 3 files

$ ~/tmp$ cat lorem/interface.go
package lorem

type Ipsum interface {
	Dollor()
}

$ ~/tmp$ go run github.com/vektra/mockery/v2@latest --all --keeptree
04 Jun 22 18:06 CEST INF Starting mockery dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST WRN !!! DEPRECATION NOTICE !!! dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST WRN Using go install to download mockery will likely break after June 1, 2022. Please view PR #456 for details. dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST INF Walking dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST INF Generating mock dry-run=false interface=Ipsum qualified-name=pkg/lorem version=v2.12.3

$ ~/tmp$ go run github.com/vektra/mockery/v2@latest --all --keeptree
04 Jun 22 18:06 CEST INF Starting mockery dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST WRN !!! DEPRECATION NOTICE !!! dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST WRN Using go install to download mockery will likely break after June 1, 2022. Please view PR #456 for details. dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST INF Walking dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST INF Generating mock dry-run=false interface=Ipsum qualified-name=pkg/lorem version=v2.12.3
04 Jun 22 18:06 CEST INF Generating mock dry-run=false interface=NewIpsumT qualified-name=pkg/mocks/lorem version=v2.12.3

$ ~/tmp$ tree
.
β”œβ”€β”€ go.mod
β”œβ”€β”€ go.sum
β”œβ”€β”€ lorem
β”‚Β Β  └── interface.go
└── mocks
    β”œβ”€β”€ lorem
    β”‚Β Β  └── Ipsum.go
    └── mocks
        └── lorem
            └── NewIpsumT.go

5 directories, 5 files

$ ~/tmp$ go run github.com/vektra/mockery/v2@latest --all --keeptree
04 Jun 22 18:06 CEST INF Starting mockery dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST WRN !!! DEPRECATION NOTICE !!! dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST WRN Using go install to download mockery will likely break after June 1, 2022. Please view PR #456 for details. dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST INF Walking dry-run=false version=v2.12.3
04 Jun 22 18:06 CEST INF Generating mock dry-run=false interface=Ipsum qualified-name=pkg/lorem version=v2.12.3
04 Jun 22 18:06 CEST INF Generating mock dry-run=false interface=NewIpsumT qualified-name=pkg/mocks/lorem version=v2.12.3
04 Jun 22 18:06 CEST INF Generating mock dry-run=false interface=NewNewIpsumTT qualified-name=pkg/mocks/mocks/lorem version=v2.12.3

$ ~/tmp$ tree
.
β”œβ”€β”€ go.mod
β”œβ”€β”€ go.sum
β”œβ”€β”€ lorem
β”‚Β Β  └── interface.go
└── mocks
    β”œβ”€β”€ lorem
    β”‚Β Β  └── Ipsum.go
    └── mocks
        β”œβ”€β”€ lorem
        β”‚Β Β  └── NewIpsumT.go
        └── mocks
            └── lorem
                └── NewNewIpsumTT.go

7 directories, 6 files
$ ~/tmp$ tree
.
β”œβ”€β”€ go.mod
β”œβ”€β”€ go.sum
└── lorem
    └── interface.go

1 directory, 3 files

$ ~/tmp$ go run github.com/vektra/mockery/v2@latest --all
04 Jun 22 17:55 CEST INF Starting mockery dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST WRN !!! DEPRECATION NOTICE !!! dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST WRN Using go install to download mockery will likely break after June 1, 2022. Please view PR #456 for details. dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST INF Walking dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST INF Generating mock dry-run=false interface=Ipsum qualified-name=pkg/lorem version=v2.12.3

$ ~/tmp$ go run github.com/vektra/mockery/v2@latest --all
04 Jun 22 17:55 CEST INF Starting mockery dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST WRN !!! DEPRECATION NOTICE !!! dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST WRN Using go install to download mockery will likely break after June 1, 2022. Please view PR #456 for details. dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST INF Walking dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST INF Generating mock dry-run=false interface=Ipsum qualified-name=pkg/lorem version=v2.12.3
04 Jun 22 17:55 CEST INF Generating mock dry-run=false interface=NewIpsumT qualified-name=pkg/mocks version=v2.12.3

$ ~/tmp$ tree
.
β”œβ”€β”€ go.mod
β”œβ”€β”€ go.sum
β”œβ”€β”€ lorem
β”‚Β Β  └── interface.go
└── mocks
    β”œβ”€β”€ Ipsum.go
    └── NewIpsumT.go

2 directories, 5 files

$ ~/tmp$ go run github.com/vektra/mockery/v2@latest --all
04 Jun 22 17:55 CEST INF Starting mockery dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST WRN !!! DEPRECATION NOTICE !!! dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST WRN Using go install to download mockery will likely break after June 1, 2022. Please view PR #456 for details. dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST INF Walking dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST ERR Error parsing file error="/home/grongor/tmp/mocks/NewIpsumT.go:8:6: NewIpsumT redeclared in this block" dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST ERR Error parsing file error="/home/grongor/tmp/mocks/NewIpsumT.go:8:6: NewIpsumT redeclared in this block" dry-run=false version=v2.12.3
04 Jun 22 17:55 CEST INF Generating mock dry-run=false interface=Ipsum qualified-name=pkg/lorem version=v2.12.3

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Version of Golang used when building/testing:

  • 1.18

How Has This Been Tested?

Run the tests + tested manually against a dummy package.

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
Contributor

Thank you so much for the PR, what a silly issue. Your proposed solution is kind of hacky but to be honest I cannot think of a better way to do it, as you need some kind of magic string to determine what interface was generated by mockery (and thus to ignore)... so this might be what we have to do.

pkg/generator.go Outdated
@@ -24,6 +24,8 @@ import (
"golang.org/x/tools/imports"
)

const mockConstructorParamTypeNamePrefix = "_testingT_6ImLfpP5pg_"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would call this mockConstructorTestingTNamePrefix. Also, what's your motivation for making this private? It seems to me like it might be useful for linters to know what this type is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, I didn't really think about it. Sure, I'll update this to be more readable (I was aiming for uniqueness).

@grongor
Copy link
Contributor Author

grongor commented Jun 14, 2022

Yeah, I was really surprised to see hundreds of "mocks" directories. 🀣

I cannot think of a better way to do it

The best way, in my opinion, would be to extend the mock.TestingT interface that we use. I'll create a PR there when this is merged.

@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2022

Codecov Report

Merging #472 (f04b040) into master (9dba1fd) will not change coverage.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##           master     #472   +/-   ##
=======================================
  Coverage   71.56%   71.56%           
=======================================
  Files           6        6           
  Lines        1252     1252           
=======================================
  Hits          896      896           
+ Misses        310      309    -1     
- Partials       46       47    +1     
Impacted Files Coverage Ξ”
pkg/walker.go 55.76% <0.00%> (-1.10%) ⬇️
pkg/generator.go 92.51% <100.00%> (ΓΈ)
cmd/mockery.go 21.49% <0.00%> (+0.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 9dba1fd...f04b040. Read the comment docs.

@@ -24,6 +24,8 @@ import (
"golang.org/x/tools/imports"
)

const mockConstructorParamTypeNamePrefix = "mockConstructorTestingT"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I actually didn't mean we couldn't use the random string idea. I'm just curious if it would make sense to make it MockConstructorTestingT instead of mockConstructorTestingT. I'm thinking mainly of linters being able to see the interface (I don't actually know if they won't see unexported interfaces). This is fine for now though, we can always change it later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, I've never written a linter for Go before :D But I'm quite sure they don't care about stuff being exported or not - that would make them a shitty linter IMO 🀣 But yeah, making it public later is easy :)

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.

None yet

3 participants