-
Notifications
You must be signed in to change notification settings - Fork 414
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 mock generation with expecter #396
Conversation
b9f0105
to
a70796d
Compare
great job. works great. |
@jimmidyson Please review this. |
@k1ng440 thanks! |
@LandonTClipp Whenever you have time, can you give this a quick look? |
This is really f-ing cool, I'm really excited to see something like this. My first intuition is that this should be part of a new major version bump. I've been considering cutting over to an alpha v3 soon and I think I'd like to have the expecter schema to be the default behavior, and not rely on backwards compatibility. It would be really cool if the One thing I was initially worried about was the fact that we are overriding methods in testify, and my concern was that we are now forced to maintain equivalence between mockery's generated functions and testify's. But I realized that you could just call the promoted field to "exit" the expecter interface and revert back to testify's call flow. I am going to need some more time to look this over and come up with a battle plan, but this has my attention. Really stellar work! |
Thanks a lot! Ping me if you have any questions regarding this feature or you need me to add/change anything. I'll take some time during the weekend to rebase the branch following the recent pull requests. |
a70796d
to
95b8d4d
Compare
After spending the last half year (!) giving it thought I finally think this is good to go through. I won't be introducing a major version bump, this will just be incorporated in v2. |
@LandonTClipp @Gevrai Why aren't the func (_e *Requester_Expecter) Get(path interface{}) *Requester_Get_Call to func (_e *Requester_Expecter) Get(path string) *Requester_Get_Call for this example. |
@johnrichardrinehart It is to still be able to use testify.Mock's functionalities for matching, such as Exact expected types are given in Expecter method's docstring. It is maybe not ideal, but I don't see a way around it, unless we get generics type on method some day (not in 1.18 even) with some changes needed on testify itself...
Open to ideas though. |
Ahhh, of course. I'm only starting to use Re generics: Yeah, unfortunately, go's generics implementation is too weak to benefit us, here. I can't offer any suggestion outside of extending the generation to produce a func (_e *Requester_Expecter) GetTyped(path string) *Requester_Get_Call which would wrap the @Gevrai would you mind pointing me to a few good public use cases where you used this this |
This feature was added very recently, I don't think it is really used in any public repos (mine isn't public sadly). Maybe the tests might be of help a bit? https://github.com/vektra/mockery/blob/master/pkg/fixtures/mocks/expecter_test.go That being said, it is expected to be used the exact same way as you would use the old I even used a simple search and replace to transform all my mocked calls in tests without issues. |
I didn't think to look at the tests 😆 . Smart! Alright, I think that's enough for me. Thanks @Gevrai ! Great work! |
@Gevrai in #406 @grongor brings up a good point. If you had some time could you fix the location of these mocks: https://github.com/vektra/mockery/tree/master/pkg/fixtures/mocks Apologies for not catching this in the original PR. Thanks again for the great work! |
TBH I did not expect this feature to be merged without more comment, code was a bit messy in some places 😅 I will try to find some time this evening to fix this and create a PR. |
@johnrichardrinehart I've created mock generator where methods are strongly typed) https://github.com/subtle-byte/mockigo |
Note: This PR is still prototypical, code is a bit messy. However, the general functionality should be stable and I'm mostly requesting for comments to see if that is something anyone would be interested on the main branch (currently using it in other projects).
All comments are more than welcomed.
Motivation for feature
Setting up mocks has always been a lot of guesswork or going back and forth between the tests and the interface's definition. The
mockery
framework already extracts all necessary information for this, it seems a logical next step to have type explicit mock construction.Goal
Adds flag
--with-expecter
which generates boilerplate code over all of relevantmocks
's functions that are missing type safety with the current way of defining mocks. Generates the usual mock boilerplate, plus anExpecter
structure for setting up calls. The naming is inspired by GoMock.Works with variadic methods, but not if the flag
--unroll-variadic=false
is set (not using it in my projects, didn't take the time to look at feasibility with it really...)Example
Given an interface such as
Generates the following as well as the usual Get mock.Called wrapper
So that one can use it like so
Pros:
mock.Anything
andmock.MatchedBy(func)
, but prints the concrete type in docstring of the Expecter method.Cons:
EXPECT
(should be easy to fix, unnecessary for RFC)mock.Call
methods are defined, if one wants to use Run or Return, it needs to be before the others, which might be a bit confusing. For exampleEXPECT().Get("").Return(...).Once().Run(...)
wouldn't compile whileEXPECT().Get("").Return(...).Run(...).Once()
would. This could be fixed by redefining all underlying methods to return the concreteCall
struct, at the cost of more boilerplate of course.Further amelioration
The
*_Expecter
and*_Call
structs could be unexported and still usable as far as I can see.Addition of a
RunAndReturn
function that takes a function with exact same signature as mocked method and defines the Return function with the result of the Run function. I ran into a potential race issue with this though and didn't go further. Might look into it more if there is a need.Make it work with
--unroll-variadic
maybe, for now it crashes. To be honest, I think that might be useless or even counter-productive though...