From bdab6673987ac44bb2e2d2ab121f7c6d46640bc4 Mon Sep 17 00:00:00 2001 From: Igor Zibarev Date: Mon, 5 Mar 2018 23:07:01 +0300 Subject: [PATCH] Fix mockgen for package name not matching import path This commit provides a fix to mockgen so it runs successfully on files that have package imports with names not matching import path suffix, e.g. package named "client" under import path "github.com/golang/mock/mockgen/tests/custom_package_name/client/v1". --- mockgen/parse.go | 40 ++++--- mockgen/parse_test.go | 108 ++++++++++++++++++ mockgen/tests/custom_package_name/README.md | 22 ++++ .../custom_package_name/client/v1/client.go | 13 +++ .../custom_package_name/greeter/greeter.go | 31 +++++ .../greeter/greeter_mock_test.go | 46 ++++++++ .../greeter/greeter_test.go | 37 ++++++ .../custom_package_name/validator/validate.go | 5 + 8 files changed, 287 insertions(+), 15 deletions(-) create mode 100644 mockgen/parse_test.go create mode 100644 mockgen/tests/custom_package_name/README.md create mode 100644 mockgen/tests/custom_package_name/client/v1/client.go create mode 100644 mockgen/tests/custom_package_name/greeter/greeter.go create mode 100644 mockgen/tests/custom_package_name/greeter/greeter_mock_test.go create mode 100644 mockgen/tests/custom_package_name/greeter/greeter_test.go create mode 100644 mockgen/tests/custom_package_name/validator/validate.go diff --git a/mockgen/parse.go b/mockgen/parse.go index 8ea62801..23509528 100644 --- a/mockgen/parse.go +++ b/mockgen/parse.go @@ -119,12 +119,14 @@ func (p *fileParser) parseAuxFiles(auxFiles string) error { if len(parts) != 2 { return fmt.Errorf("bad aux file spec: %v", kv) } - file, err := parser.ParseFile(p.fileSet, parts[1], nil, 0) + pkg, fpath := parts[0], parts[1] + + file, err := parser.ParseFile(p.fileSet, fpath, nil, 0) if err != nil { return err } p.auxFiles = append(p.auxFiles, file) - p.addAuxInterfacesFromFile(parts[0], file) + p.addAuxInterfacesFromFile(pkg, file) } return nil } @@ -138,6 +140,8 @@ func (p *fileParser) addAuxInterfacesFromFile(pkg string, file *ast.File) { } } +// parseFile loads all file imports and auxiliary files import into the +// fileParser, parses all file interfaces and returns package model. func (p *fileParser) parseFile(file *ast.File) (*model.Package, error) { allImports := importsOfFile(file) // Don't stomp imports provided by -imports. Those should take precedence. @@ -170,6 +174,8 @@ func (p *fileParser) parseFile(file *ast.File) (*model.Package, error) { }, nil } +// parsePackage loads package specified by path, parses it and populates +// corresponding imports and importedInterfaces into the fileParser. func (p *fileParser) parsePackage(path string) error { var pkgs map[string]*ast.Package if imp, err := build.Import(path, p.srcDir, build.FindOnly); err != nil { @@ -417,30 +423,34 @@ func (p *fileParser) parseType(pkg string, typ ast.Expr) (model.Type, error) { // importsOfFile returns a map of package name to import path // of the imports in file. func importsOfFile(file *ast.File) map[string]string { - /* We have to make guesses about some imports, because imports are not required - * to have names. Named imports are always certain. Unnamed imports are guessed - * to have a name of the last path component; if the last path component has dots, - * the first dot-delimited field is used as the name. - */ - m := make(map[string]string) for _, is := range file.Imports { - var pkg string + var pkgName string importPath := is.Path.Value[1 : len(is.Path.Value)-1] // remove quotes if is.Name != nil { + // Named imports are always certain. if is.Name.Name == "_" { continue } - pkg = removeDot(is.Name.Name) + pkgName = removeDot(is.Name.Name) } else { - _, last := path.Split(importPath) - pkg = strings.SplitN(last, ".", 2)[0] + pkg, err := build.Import(importPath, "", 0) + if err != nil { + // Fallback to import path suffix. Note that this is uncertain. + log.Printf("failed to import package by path %s: %s - fallback to import path suffix", importPath, err.Error()) + _, last := path.Split(importPath) + // If the last path component has dots, the first dot-delimited + // field is used as the name. + pkgName = strings.SplitN(last, ".", 2)[0] + } + pkgName = pkg.Name } - if _, ok := m[pkg]; ok { - log.Fatalf("imported package collision: %q imported twice", pkg) + + if _, ok := m[pkgName]; ok { + log.Fatalf("imported package collision: %q imported twice", pkgName) } - m[pkg] = importPath + m[pkgName] = importPath } return m } diff --git a/mockgen/parse_test.go b/mockgen/parse_test.go new file mode 100644 index 00000000..f455ade4 --- /dev/null +++ b/mockgen/parse_test.go @@ -0,0 +1,108 @@ +package main + +import ( + "go/ast" + "go/parser" + "go/token" + "testing" +) + +func TestFileParser_ParseFile(t *testing.T) { + fs := token.NewFileSet() + file, err := parser.ParseFile(fs, "tests/custom_package_name/greeter/greeter.go", nil, 0) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + p := fileParser{ + fileSet: fs, + imports: make(map[string]string), + importedInterfaces: make(map[string]map[string]*ast.InterfaceType), + } + + pkg, err := p.parseFile(file) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + checkGreeterImports(t, p.imports) + + expectedName := "greeter" + if pkg.Name != expectedName { + t.Fatalf("Expected name to be %v but got %v", expectedName, pkg.Name) + } + + expectedInterfaceName := "InputMaker" + if pkg.Interfaces[0].Name != expectedInterfaceName { + t.Fatalf("Expected interface name to be %v but got %v", expectedInterfaceName, pkg.Interfaces[0].Name) + } +} + +func TestFileParser_ParsePackage(t *testing.T) { + fs := token.NewFileSet() + _, err := parser.ParseFile(fs, "tests/custom_package_name/greeter/greeter.go", nil, 0) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + p := fileParser{ + fileSet: fs, + imports: make(map[string]string), + importedInterfaces: make(map[string]map[string]*ast.InterfaceType), + } + + err = p.parsePackage("github.com/golang/mock/mockgen/tests/custom_package_name/greeter") + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + checkGreeterImports(t, p.imports) +} + +func TestImportsOfFile(t *testing.T) { + fs := token.NewFileSet() + file, err := parser.ParseFile(fs, "tests/custom_package_name/greeter/greeter.go", nil, 0) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + imports := importsOfFile(file) + checkGreeterImports(t, imports) +} + +func checkGreeterImports(t *testing.T, imports map[string]string) { + // check that imports have stdlib package "fmt" + if fmtPackage, ok := imports["fmt"]; !ok { + t.Errorf("Expected imports to have key \"fmt\"") + } else { + expectedFmtPackage := "fmt" + if fmtPackage != expectedFmtPackage { + t.Errorf("Expected fmt key to have value %s but got %s", expectedFmtPackage, fmtPackage) + } + } + + // check that imports have package named "validator" + if validatorPackage, ok := imports["validator"]; !ok { + t.Errorf("Expected imports to have key \"fmt\"") + } else { + expectedValidatorPackage := "github.com/golang/mock/mockgen/tests/custom_package_name/validator" + if validatorPackage != expectedValidatorPackage { + t.Errorf("Expected validator key to have value %s but got %s", expectedValidatorPackage, validatorPackage) + } + } + + // check that imports have package named "client" + if clientPackage, ok := imports["client"]; !ok { + t.Errorf("Expected imports to have key \"client\"") + } else { + expectedClientPackage := "github.com/golang/mock/mockgen/tests/custom_package_name/client/v1" + if clientPackage != expectedClientPackage { + t.Errorf("Expected client key to have value %s but got %s", expectedClientPackage, clientPackage) + } + } + + // check that imports don't have package named "v1" + if _, ok := imports["v1"]; ok { + t.Errorf("Expected import not to have key \"v1\"") + } +} diff --git a/mockgen/tests/custom_package_name/README.md b/mockgen/tests/custom_package_name/README.md new file mode 100644 index 00000000..b16da215 --- /dev/null +++ b/mockgen/tests/custom_package_name/README.md @@ -0,0 +1,22 @@ +# Tests for custom package names + +This directory contains test for mockgen generating mocks when imported package +name does not match import path suffix. For example, package with name "client" +is located under import path "github.com/golang/mock/mockgen/tests/custom_package_name/client/v1". + +Prior to this patch: + + $ go generate greeter/greeter.go + 2018/03/05 22:44:52 Loading input failed: greeter.go:17:11: failed parsing returns: greeter.go:17:14: unknown package "client" + greeter/greeter.go:1: running "mockgen": exit status 1 + +This can be fixed by manually providing `-imports` flag, like `-imports client=github.com/golang/mock/mockgen/tests/custom_package_name/client/v1`. +But, mockgen should be able to automatically resolve package names in such situations. + +With this patch applied: + + $ go generate greeter/greeter.go + $ echo $? + 0 + +Mockgen runs successfully, produced output is equal to [greeter_mock_test.go](greeter/greeter_mock_test.go) content. diff --git a/mockgen/tests/custom_package_name/client/v1/client.go b/mockgen/tests/custom_package_name/client/v1/client.go new file mode 100644 index 00000000..fc799d9c --- /dev/null +++ b/mockgen/tests/custom_package_name/client/v1/client.go @@ -0,0 +1,13 @@ +package client + +import "fmt" + +type Client struct{} + +func (c *Client) Greet(in GreetInput) string { + return fmt.Sprintf("Hello, %s!", in.Name) +} + +type GreetInput struct { + Name string +} diff --git a/mockgen/tests/custom_package_name/greeter/greeter.go b/mockgen/tests/custom_package_name/greeter/greeter.go new file mode 100644 index 00000000..8c2d2937 --- /dev/null +++ b/mockgen/tests/custom_package_name/greeter/greeter.go @@ -0,0 +1,31 @@ +//go:generate mockgen -source greeter.go -destination greeter_mock_test.go -package greeter + +package greeter + +import ( + // stdlib import + "fmt" + + // non-matching import suffix and package name + "github.com/golang/mock/mockgen/tests/custom_package_name/client/v1" + + // matching import suffix and package name + "github.com/golang/mock/mockgen/tests/custom_package_name/validator" +) + +type InputMaker interface { + MakeInput() client.GreetInput +} + +type Greeter struct { + InputMaker InputMaker + Client *client.Client +} + +func (g *Greeter) Greet() (string, error) { + in := g.InputMaker.MakeInput() + if err := validator.Validate(in.Name); err != nil { + return "", fmt.Errorf("validation failed: %v", err) + } + return g.Client.Greet(in), nil +} diff --git a/mockgen/tests/custom_package_name/greeter/greeter_mock_test.go b/mockgen/tests/custom_package_name/greeter/greeter_mock_test.go new file mode 100644 index 00000000..0bf0f46f --- /dev/null +++ b/mockgen/tests/custom_package_name/greeter/greeter_mock_test.go @@ -0,0 +1,46 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: greeter.go + +// Package greeter is a generated GoMock package. +package greeter + +import ( + gomock "github.com/golang/mock/gomock" + v1 "github.com/golang/mock/mockgen/tests/custom_package_name/client/v1" + reflect "reflect" +) + +// MockInputMaker is a mock of InputMaker interface +type MockInputMaker struct { + ctrl *gomock.Controller + recorder *MockInputMakerMockRecorder +} + +// MockInputMakerMockRecorder is the mock recorder for MockInputMaker +type MockInputMakerMockRecorder struct { + mock *MockInputMaker +} + +// NewMockInputMaker creates a new mock instance +func NewMockInputMaker(ctrl *gomock.Controller) *MockInputMaker { + mock := &MockInputMaker{ctrl: ctrl} + mock.recorder = &MockInputMakerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockInputMaker) EXPECT() *MockInputMakerMockRecorder { + return m.recorder +} + +// MakeInput mocks base method +func (m *MockInputMaker) MakeInput() v1.GreetInput { + ret := m.ctrl.Call(m, "MakeInput") + ret0, _ := ret[0].(v1.GreetInput) + return ret0 +} + +// MakeInput indicates an expected call of MakeInput +func (mr *MockInputMakerMockRecorder) MakeInput() *gomock.Call { + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MakeInput", reflect.TypeOf((*MockInputMaker)(nil).MakeInput)) +} diff --git a/mockgen/tests/custom_package_name/greeter/greeter_test.go b/mockgen/tests/custom_package_name/greeter/greeter_test.go new file mode 100644 index 00000000..f056ec81 --- /dev/null +++ b/mockgen/tests/custom_package_name/greeter/greeter_test.go @@ -0,0 +1,37 @@ +package greeter + +import ( + "testing" + + "github.com/golang/mock/gomock" + "github.com/golang/mock/mockgen/tests/custom_package_name/client/v1" +) + +func TestGreeter_Greet(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + input := client.GreetInput{ + Name: "Foo", + } + + inputMaker := NewMockInputMaker(ctrl) + inputMaker.EXPECT(). + MakeInput(). + Return(input) + + g := &Greeter{ + InputMaker: inputMaker, + Client: &client.Client{}, + } + + greeting, err := g.Greet() + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + expected := "Hello, Foo!" + if greeting != expected { + t.Fatalf("Expected greeting to be %v but got %v", expected, greeting) + } +} diff --git a/mockgen/tests/custom_package_name/validator/validate.go b/mockgen/tests/custom_package_name/validator/validate.go new file mode 100644 index 00000000..0617c4be --- /dev/null +++ b/mockgen/tests/custom_package_name/validator/validate.go @@ -0,0 +1,5 @@ +package validator + +func Validate(s string) error { + return nil +}