FEEDBACK REQUESTED: packages feature
#549
Replies: 14 comments 56 replies
-
|
The proposed changes are ok to me, i have only one question about the performance. This will not change the performance of the generated code, but the performance of the generator. So the question is who does really care about the generator's performance? I mean that i usually run it once in a while and commit the generated code to a repo. So go ahead and don't forget to provide a documentation and/or examples. |
Beta Was this translation helpful? Give feedback.
-
|
I'm ok with removing support for recursively discovering interfaces, but it's pretty useful to have automatic discovery of interfaces within a package. For example, I have some generated clients from protobuf specs, and generate the mocks of those client interfaces for unit testing. Manually adding mocks for each interface is tedious, particularly as there's dozens of clients. A little bit of magic in detection here is much appreciated. With regards to the finer control of which interfaces get mocks, there are times when I have mocks I don't want or need. But I'd say those undesirable mocks are in the minority, and I'm ok with them knocking around. The only ones that are slightly irksome are mocks for private interfaces that are output into a different package and so can't ever be imported. But those aren't a huge issue, and it looks like I could rename them with the proposed config anyway. |
Beta Was this translation helpful? Give feedback.
-
|
I'm testing this feature, one thing that I am missing is that currently my mocks are inside a "mocks" directory inside each package folder, but this don't seem to be possible currently, maybe adding a "PackageFolder" option to the templated directory and file name? Current |
Beta Was this translation helpful? Give feedback.
-
|
Playing around with this feature, in general it would be very useful to support some kind of defaults for all packages so that you do not have to repeat that for every package. For example now i need to specify |
Beta Was this translation helpful? Give feedback.
-
|
For us, all public interfaces need mocks. We are generating all mocks in a separate mockery --all --keeptree --inpackage
rm -fr mocks/unneededThis change sounds good, and will help better define which mocks we need and which we don't. But we rely completely on the recursive discovery. Also performance is not a problem because of the reduced size of our services, so removing recursion adds no benefit for us. We would wait until something like this is added (like @SVilgelm suggested) before migrating: packages:
github.com/vektra/mockery/v2/pkg:
config:
all: True
recursive: TrueOr at least until you add something like @jbub proposal of supporting templates somehow. In general, being forced to specify that for every package would be a usability problem for us. |
Beta Was this translation helpful? Give feedback.
-
|
I don't understand why this downgrade. Why do I need a config now? This worked perfectly as one-liner for go generate e.g. I love the lib so far, but the incoming changes seem to be a no-go for me. |
Beta Was this translation helpful? Give feedback.
-
|
One small pain point I felt is that we have a monorepo with many microservices and CODEOWNERS permissions for code review. If I leave the entire mock configuration on the root of the repo, then people with root folder access would need to always review these changes, when today only the team responsible for the service needs to approve on a mock generation change. It would be great if there was some way of "importing" configuration from other folders, but then we would have the problem of relative paths, and I look at the code, it doesn't seem to be easy to add such a thing. I'll need to think of how to handle this. |
Beta Was this translation helpful? Give feedback.
-
|
After initial rant, this is my current config. I'm testing on version 2.22.1. log-level: warn
disable-version-string: True
all: True
exported: True
with-expecter: True
output: ./internal/mocksThis works well with //go:generate mockery
package mainBut fails horribly if I ran I've tried log-level: warn
disable-version-string: True
all: True
exported: True
with-expecter: True
output: "{{.InterfaceDir}}/internal/mocks"🙈 Would be nice to get it implemented ;d |
Beta Was this translation helpful? Give feedback.
-
|
In my own usecase I'd also be interested in keeping the recursive autodiscovery, we have a large project with a lot of subpackages... and if we're forced to use the packages configuration we'd probably have to write a tool ourselves to generate the configuration by walking our pkg folder... |
Beta Was this translation helpful? Give feedback.
-
|
I also tried to implement the current packages method in my monorepo with multiple services, but as @a-pedini , having to add folder-by-folder (we use something like DDD, so each service has a lot of separation), wasn't really an improvement, I ended up with lots of 30+ line configurations, when just one "recursive" option per service would suffice. Maybe an option of using in-source directives for each object to mock? Like but I think a global autodiscovery would be good enough. |
Beta Was this translation helpful? Give feedback.
-
|
Been playing with v2.25.0 and it seems like the last puzzle missing for my usecase is the ability to change the mock name based on whether the original interface is exported/unexported. Maybe adding some flag to the template context would help: |
Beta Was this translation helpful? Give feedback.
-
|
@arvenil @RangelReale @jbub @rubencaro @mattdowdell @SVilgelm I've put a bunch of effort into implementing the suggestions that have raised in this discussion. So far we have:
Let me know what you think of the current iteration and if there is anything else you would want added. I decided that I'm not going to immediately implement the If there's a general consensus on the current form, I'll take the feature into a beta state, where it'll probably reside for at least a few months. |
Beta Was this translation helpful? Give feedback.
-
|
I implemented this in my monorepo, because of the CODEOWNERS issue I had to create a config file per microservice, didn't use the It's much more organized this way also, we have more control of the generation process. |
Beta Was this translation helpful? Give feedback.
-
|
Closing this discussion as the feature is quite mature now. Thanks for the discussions everyone. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Update
2023-07-10
packageshas been fully released! Thanks for the feedback and insights into how you use mockery. We can finally put this saga behind us and move on to more exciting ideas I have in mind.2023-06-21
We are wrapping up with the
packagesfeature. It has been in beta state with minimal issue for over a month now. When the time is right, I will consider this feature to be production ready.Please view my blog post showcasing this new feature here: https://landontclipp.github.io/blog/2023/06/21/introducing-mockerys-new-packages-feature/
2023-05-14
As of v2.27.1, the
packagesfeature has been moved to a beta state. We will leave it in this state for the next couple of months to allow for beta testing. Once it is moved out of beta, we will create a deprecation notice for the legacy configuration schema.2023-04-06
The community has made it clear that automatic package discovery is a requirement as many projects have a large number of sub-packages. We will implement automatic package discovery before deprecating the old configuration style and removing the
packagesfeature from alpha.2023-03-09
As of
v2.21.1, we now have thepackagesconfig section that can be used. See details of it here: https://vektra.github.io/mockery/features/#packages-configurationLet me know your feedback and if there are any changes you'd like to be made! Note this feature is ALPHA, so it will likely have bugs and may have additions or subtractions to its feature set.
Description
We are adding a new feature in #548 that allows specifying explicit packages to generate mocks for, instead of the old method of dynamically discovering files on the filesystem using
--all=True. The issue with the current style is multifaceted:mockery --all, mockery will recursively iterate the filesystem and for each go file it finds, it will callpackages.Loadon just that file. This means thatpackages.Loadwill be called multiple times for every file in your package, which is an obvious redundancy. You could potentially fix this by caching thepackage.Loadresult on a per-directory, instead of a per-file level. This would cache thepackage.Loadcall once for a directory instead of multiple times, which could provide a performance benefit. However, the higher level picture is that dynamically discovering interfaces to mock is likely wasteful, as it's unlikely you would legitimately use every mock you've created.//go:generatedirective is similarly wasteful under the current approach. Folks who use generate can likely get around some of the performance issues of (1) because they are generating mocks for a much smaller set of interfaces. For example, let's say you have a package with 5 files in it and 6 interfaces. If you usedgenerateon all 6 of those interfaces, you'd be invoking mockery 6 separate times, which would redundantly callpackages.Load6 times. This would be a worse situation calling mockery once per file!What we propose is a yaml file like this:
The config provided to each interface would override package-level config, which would override defaults at the top-level.
This model of configuration is extremely powerful and opens a lot of doors for a high degree of customizability and flexibility.
You could emulate
--allby doing:However this would still only generate mocks for
github.com/vektra/mockery/v2/pkg, not recursively iterate to sub packages.Benefits this grants us
packages.Loadis called once for each package, instead of once per file in current schemeProposed Deprecation
We propose deprecating
mockery --all, as this uses legacypackage.Loadbehavior that causes slow mock generation. Additionally, deprecating this means we can remove a ton of legacy code that is a relic of the pre-go-module era. Thepackagesconfig provides sufficient means for generating mocks for a package, which makes--allredundant.packagesalso adds a ton of new features that we get almost for free, and back porting those features to--allis an inefficient use of developer time.Caveat
The one caveat of this is that it might make
go:generateredundant. If you use thepackagesconfig in your repo but also use//go:generate mockery --name Stringerat some point in your repo, mockery would need to be explicitly aware of the fact that it's running under ageneratedirective and ignore thepackagesconfig. Otherwise it would try to generate all the packages you listed on every invocation ofgenerate.Questions To The Community
go:generatemethod of generating mocks?Current PR
#548
Beta Was this translation helpful? Give feedback.
All reactions