matryer / moq

Interface mocking tool for go generate
http://bit.ly/meetmoq
MIT License
1.96k stars 126 forks source link

Fix: add imports for Named Type its Instance Types #199

Closed TimVosch closed 1 year ago

TimVosch commented 1 year ago

Fixes missing imports and package prefixing to generic types. See #177

In the following example a method must be mocked which has a return parameter that has a generic type: otherpackage.Foo.

package genericreturn

import "github.com/matryer/moq/pkg/moq/testpackages/genericreturn/otherpackage"

type GenericBar[T any] struct {
    Bar T
}

type IFooBar interface {
    Foobar() GenericBar[otherpackage.Foo]
}
package otherpackage

type Foo struct {
    A int
    B string
}

Currently mocking IFooBar will result in a missing otherpackage import and the package prefix to Foo. Note: FoobarFunc: func() GenericBar[Foo] instead of FoobarFunc: func() GenericBar[otherpackage.Foo].

// IFooBarMock is a mock implementation of IFooBar.
//
//  func TestSomethingThatUsesIFooBar(t *testing.T) {
//
//      // make and configure a mocked IFooBar
//      mockedIFooBar := &IFooBarMock{
//          FoobarFunc: func() GenericBar[Foo] {
//              panic("mock out the Foobar method")
//          },
//      }
//
//      // use mockedIFooBar in code that requires IFooBar
//      // and then make assertions.
//
//  }

This happens because of an invalid key lookup here, which returns an empty string "": internal/registry/var.go

This is fixed by checking if a Named Type has type arguments, and populating imports for those types. internal/registry/method_scope.go

func (m MethodScope) populateImports(t types.Type, imports map[string]*Package) {
    switch t := t.(type) {
    case *types.Named:
        ...
        // The imports of a Type with a TypeList must be added to the imports list
        // For example: Foo[otherpackage.Bar] , must have otherpackage imported
        if targs := t.TypeArgs(); targs != nil {
            for i := 0; i < targs.Len(); i++ {
                m.populateImports(targs.At(i), imports)
            }
        }

A new golden test was added for this specific case.

Closes #177 Closes #190

sudo-suhas commented 1 year ago

Expected error message in the TestParseError test is couldn't load source package: ~/moq/pkg/moq/testpackages/_parseerror/service/service.go:3:8: could not import github.com/matryer/notexist (invalid package name: "").

Not sure why the error is couldn't load source package: err: exit status 1: stderr: go build github.com/matryer/notexist: service.go:3:8: cannot find package "." in: ~/moq/pkg/moq/testpackages/vendor/github.com/matryer/notexist only on Go 1.19.

TimVosch commented 1 year ago

~Hmm, this also appears to happen on the main branch. I expect it's an issue unrelated to this PR. I'll see if I can find something.~

``` ╭─timvosch@XPS15-ARCH ~/src/moq ‹373b8f9› ╰─➤ docker run --rm -it -v `pwd`:/project --workdir /project golang:1.19 go test ./... go: downloading github.com/pmezard/go-difflib v1.0.0 go: downloading golang.org/x/tools v0.3.0 go: downloading golang.org/x/mod v0.7.0 go: downloading golang.org/x/sys v0.2.0 ? github.com/matryer/moq [no test files] ok github.com/matryer/moq/example 0.001s [no tests to run] ? github.com/matryer/moq/generate [no test files] ? github.com/matryer/moq/internal/registry [no test files] ok github.com/matryer/moq/internal/template 0.001s --- FAIL: TestGoGenerateVendoredPackages (0.00s) moq_test.go:629: Wait: exit status 1 --- FAIL: TestParseError (0.01s) moq_test.go:663: unexpected error: couldn't load source package: err: exit status 1: stderr: go build github.com/matryer/notexist: service.go:3:8: cannot find package "." in: /project/pkg/moq/testpackages/vendor/github.com/matryer/notexist FAIL FAIL github.com/matryer/moq/pkg/moq 6.336s FAIL ```

Edit: Actually installing the moq package in the docker container runs the tests on main succesfully now, but only for <= 1.19.9

docker run --rm -it -v `pwd`:/project --workdir /project golang:1.19.0 sh -c 'go version && git --version && go install -buildvcs=false . && go test ./...'

Specifically version 1.19.10 does not work. All minor revisions below that work. Since it was released just two weeks ago, that is why it probably wasn't picked up earlier. I am going through the changelog to see why. I expected the error message just changed and the test needs an update.

docker run --rm -it -v `pwd`:/project --workdir /project golang:1.19.10 sh -c 'git -v && go install -buildvcs=false . && go test ./...'

Edit 2: Between go 1.19.9 and 1.19.10 the following change happened. I expect that the test should expect a build error instead of the import error, but only for go 1.19.10.

diff --git a/src/cmd/go/internal/work/exec.go b/src/cmd/go/internal/work/exec.go
index c24d210711..567f045922 100644
--- a/src/cmd/go/internal/work/exec.go
+++ b/src/cmd/go/internal/work/exec.go
@@ -528,6 +528,12 @@ func (b *Builder) build(ctx context.Context, a *Action) (err error) {
                b.Print(a.Package.ImportPath + "\n")
        }

+       if p.Error != nil {
+               // Don't try to build anything for packages with errors. There may be a
+               // problem with the inputs that makes the package unsafe to build.
+               return p.Error
+       }
+
        if a.Package.BinaryOnly {
                p.Stale = true
                p.StaleReason = "binary-only packages are no longer supported"

@sudo-suhas I am unsure what to do with this, do you have an idea?

sudo-suhas commented 1 year ago

We can add an additional clause for the error string check to cover errors from both Go 1.18 and Go 1.19.

TimVosch commented 1 year ago

I updated the test to also check for stderr: go build github.com/matryer/notexist

sudo-suhas commented 1 year ago

This change has been released in v0.3.2 🎉