Fix(recursive): use packages.Load support for recursive search instea…

sonalys commented 1 week ago

Description

Related Issue: #636

This Pull Request attempts to implement a better recursive behavior for mockery. Instead of giving an error when no Go files are present, we lookup using packages.Load and ignore packages with no goFiles.

Shifts sub-package lookup responsibility to packages.Load. I had to change the tests to not error when there are no go files for the package.

I would love some feedback about this proposal.

Type of change

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
[ ] This change requires a documentation update

Version of Go used when building/testing:

[ ] 1.22
[x] 1.23

How Has This Been Tested?

It was ran against a package that contains no Go files, but do contain sub-packages. These sub-packages had the mocked files generated without any further issues.

Checklist

[x] My code follows the style guidelines of this project
[x] 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
[x] My changes generate no new warnings
[ ] I have added tests that prove my fix is effective or that my feature works
[x] New and existing unit tests pass locally with my changes

LandonTClipp commented 1 week ago

Thanks for the PR. Does this work in the case where the package exists on a remote git repo (not on local disk)?

sonalys commented 1 week ago

Thanks for the PR. Does this work in the case where the package exists on a remote git repo (not on local disk)?

Could you help me test that?

I have tested on a package that generates mocks from the own package and from an external package ( a github one ).

I don't know exactly what you mean by local disk. The package is cached at the go cache folder and referenced using the package name, not a filepath

LandonTClipp commented 1 week ago

I tested this out myself. I forked the AWS Go SDK, modified it a bit, and attempted to generate mocks for a package with no Go files, using your changes.

https://github.com/LandonTClipp/aws-sdk-go/tree/main/service

I added this config to the .mockery.yaml file of the mockery project itself:

  github.com/LandonTClipp/aws-sdk-go/service:
    config:
      recursive: True
      all: True
      exclude: ["cloudwatch", "s3", "internal", "s3control", "glacier"]

I deleted the .go file to test if mockery can still load the packages:

07 Nov 24 17:19 CST DBG number of sub-packages found dry-run=false num-subpackages=772 package-path=github.com/LandonTClipp/aws-sdk-go/service version=v0.0.0-dev

Aha! It found 772, which sounds about right. (Note you need to append "/..." not "..." to the package name.) This looks promising, I will do more testing tomorrow probably.

LandonTClipp commented 1 week ago

I tried another example with a more minimal Go module: https://github.com/LandonTClipp/example-go

  github.com/LandonTClipp/example-go/pkg:
    config:
      recursive: True
      all: True

[  6:59PM ]  [ landon@A02257:~/git/vektra/mockery(master✗) ]
 $ go version             
go version go1.23.2 darwin/arm64
[  6:59PM ]  [ landon@A02257:~/git/vektra/mockery(master✗) ]
 $ task mocks             
task: [mocks.generate] go run .
task: [mocks.remove] find . -name '*_mock.go' | xargs -r rm
task: [mocks.remove] rm -rf mocks/
07 Nov 24 20:26 CST INF Starting mockery dry-run=false version=v0.0.0-dev
07 Nov 24 20:26 CST INF Using config: /Users/landon/git/vektra/mockery/.mockery.yaml dry-run=false version=v0.0.0-dev
07 Nov 24 20:26 CST INF done loading, visiting interface nodes dry-run=false version=v0.0.0-dev
[...]

It's successful! And I see the mocks outputted.

Screenshot 2024-11-07 at 8 27 12 PM

So yeah I guess I was totally wrong. I thought I had tried this initially, maybe the Go compiler handles this better now? Idk, but clearly your fix works AND it gets rid of a lot of nasty code I had to write. Well done!

Another thing to think about. Do we really need the recursive option at all? Can we not just specify github.com/LandonTClipp/example-go/pkg/... and get the same behavior?

Yet another thing to think about.... currently when mockery discovers the recursive packages, it injects configuration into the yaml (well really, just the in-memory map) for each subpackage as if it was specified in the yaml. You can see this in the mockery showconfig command:

packages:
  github.com/LandonTClipp/example-go/pkg:
    config:
      all: true
      disable-version-string: true
      filename: '{{.MockName}}.go'
      mockname: '{{.InterfaceName}}'
      outpkg: mocks
      quiet: false
      recursive: true
      tags: custom2
      with-expecter: true
  github.com/LandonTClipp/example-go/pkg/subpkg1:
    config:
      all: true
      disable-version-string: true
      filename: '{{.MockName}}.go'
      mockname: '{{.InterfaceName}}'
      outpkg: mocks
      quiet: false
      recursive: true
      tags: custom2
      with-expecter: true
  github.com/LandonTClipp/example-go/pkg/subpkg2:
    config:
      all: true
      disable-version-string: true
      filename: '{{.MockName}}.go'
      mockname: '{{.InterfaceName}}'
      outpkg: mocks
      quiet: false
      recursive: true
      tags: custom2
      with-expecter: true

I wonder if we were to rely on the ... notation instead of recursive, maybe this is not necessary?

LandonTClipp commented 3 days ago

Great! Thank you, I will get this released soon.

vektra / mockery