-
Notifications
You must be signed in to change notification settings - Fork 413
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
Add a replace-type parameter to allow replacing package and/or type names #540
Conversation
So I might be missing something here, but it does seem to me like the parsing packages Golang provides would indeed be able to differentiate type alias from the underlying type, as shown by my proof of concept in the linked issue. Although I wonder if the issue is that the The The question becomes, is the If it is possible to extract alias names, I would much prefer that solution than having the burden be placed on users to create manual exceptions to type names. I have a suspicion mockery is simply doing something wrong here. |
I looked at the issues of other mock generator and they all have the same problem, see these 2 gomock issues for example: mockgen should not follow type aliases Looks like they created an alternative "source" mode to parse the source directly to avoid missing this information. I stepped on the code and looked at the object returned to the type alias, I didn't find any mention of the alias anywhere, the same that is said on the first issue above. So with the current mockery version it is effectivelly impossible to overcome the |
What I would like to determine before committing to this proposal is what part of the mockery call stack do we lose the alias information? I'm betting that the breakage is happening somewhere in this method: Line 59 in 8641a5b
but I want to confirm for certain that there's nothing we could do here. At the moment it's not clear to me at what point the alias information is lost. I'm sure we could add a bunch of print statements in the code to prove it... it would help my sanity. |
It is easy to step in the code using this MR to trigger this problem, step into the |
There's a gap in your proof-of-concept. You're correct in that you can identify a type alias declaration. However, aliases are not preserved in the type graph, so any variables, struct fields, etc. of an alias type are resolved as the aliased type itself and any useful information about the alias itself is discarded and unavailable from those nodes. You can't distinguish between the alias and the type to which it refers when using This is the closest thing to an upstream tracking issue for this problem that I can find: golang/go#44410 I'd love to have the proposed functionality as an option until the Go language itself fixes the underlying issue, and I'm not holding my breath waiting for that to happen. |
@wyattanderson that makes more sense. I understand the need for this a little better. It's unfortunate and I really hate having to maintain hacky stuff like this but I can't think of a better solution until the go maintainers fix the parsers (which probably isn't anywhere high on their priority list). After having given it more thought I think we can go ahead with this. However @RangelReale could you add a section in the mkdocs page here: https://vektra.github.io/mockery/features/ . I am not adding any documentation into the root readme anymore. We can just add another section regarding type aliases and how to resolve it with the feature you've implemented here. |
c152fc0
to
3589265
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #540 +/- ##
===================================================
+ Coverage 74.92926% 75.35754% +0.42828%
===================================================
Files 7 7
Lines 1767 1818 +51
===================================================
+ Hits 1324 1370 +46
- Misses 358 362 +4
- Partials 85 86 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@LandonTClipp yes I fully agree with you, I hate having to add these kinds of workarounds, but in this case there seems to be absolutely no way around it. I'll look at cleaning up the code and updating the documentation tomorrow. @wyattanderson @LandonTClipp maybe we can detect that this is an alias and issue a warning about it? |
3589265
to
17b2136
Compare
I refactored the parameter parsing to be done only at startup, see if everything is ok. |
Great work, I’ll give it a review in the next day or so. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some changes I want to make, one of which will hopefully simplify the unit test a bit. Let me know what you think of those.
pkg/generator_test.go
Outdated
@@ -2411,6 +2411,93 @@ import mock "github.com/stretchr/testify/mock" | |||
s.checkPrologueGeneration(generator, expected) | |||
} | |||
|
|||
func (s *GeneratorSuite) TestInternalPackagePrologue() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename this "TestReplaceTypePackagePrologue"
pkg/generator_test.go
Outdated
mock.Mock | ||
} | ||
|
||
// DoFoo provides a mock function with given fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to move away from this type of test where we assert exactly the code that is generated... it's fragile because if you make one change to the mock generation then you have to modify dozens of tests.
Maybe a better way to do this is to write the mock in-memory and simply assert that *baz.Baz
exists and *foo.InternalBaz
does not exist in the generated string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a regexp check function and used it for this assertion.
Thank you sir, I'll take another look at this in the next day or so! |
Very late to the game but hit this error and I have a problem that this does not solve. I have multiple packages that
|
@papadeltasierra thats really tricky. Unfortunately the only way you can get around this is by manually specifying the replacements for each individual type. There is no way mockery can know where your type alias originally came from because historically, a type alias is not represented in the type tree. There is a lead I'm chasing up in Go 1.22 where we might be able to retain the alias information if you set a specific GODEBUG flag. The Go authors know this is an issue and have made some half attempt at providing a fix. |
@LandonTClipp thanks for the info. Currently I've manually "patched" the mockery output and removed this interface from |
@papadeltasierra yes the high level explanation is that prior to Go 1.22, the parsed type tree did not have any representation for a type alias. It always got resolved down to the underlying type, so there was no way to extract the alias information. This was a limitation with the Go package parser itself, not with mockery. You can see in 1.22 they added a |
@papadeltasierra update on the |
Description
Add a
replace-type
parameter to allow replacing package and/or type names.This overcomes the problem of type aliases to internal packages, like
cloud.google.com/go/pubsub.Message
, which is a type alias, for which the mock forwarded the implementation to an internal package that's not exported.Type of change
Version of Golang used when building/testing:
How Has This Been Tested?
Tests included on the MR.
Tested with local code that imported
cloud.google.com/go/pubsub.Message
.Checklist