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(recursive): use packages.Load support for recursive search instea… #838

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

sonalys
Copy link
Contributor

@sonalys sonalys commented Nov 7, 2024

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

  • 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
  • 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

  • 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
Collaborator

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

@sonalys
Copy link
Contributor Author

sonalys commented Nov 7, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

LandonTClipp commented Nov 8, 2024

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?

pkg/config/config.go Outdated Show resolved Hide resolved
@sonalys sonalys force-pushed the fix/recursive branch 2 times, most recently from 226f24d to 4192ae1 Compare November 8, 2024 06:57
…d of manual

- Fix(Parsepackages): Ignore packages with no Go files
@LandonTClipp LandonTClipp merged commit 057cf5e into vektra:master Nov 13, 2024
4 checks passed
@LandonTClipp
Copy link
Collaborator

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

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.

2 participants