From 85325bc95035528731b500359c3bfc73a64f6365 Mon Sep 17 00:00:00 2001 From: Oleg Butuzov Date: Mon, 15 May 2023 10:59:32 +0300 Subject: [PATCH] chore: add more tests & API Change (#22) * test: `Imports` * test: added `Violation` tests * test: added golangci `(*Violation).Issue()` tests + changed API --- go.mod | 8 +- go.sum | 17 ++ internal/checker/imports_test.go | 94 +++++++ internal/checker/testdata/imports_alias.txtar | 12 + internal/checker/testdata/imports_all.txtar | 55 +++++ internal/checker/testdata/imports_dot.txtar | 12 + internal/checker/testdata/imports_many.txtar | 22 ++ .../checker/testdata/imports_nothing.txtar | 5 + .../checker/testdata/imports_regular.txtar | 12 + .../testdata/violations_new_lines.txtar | 12 + .../testdata/violations_packages.txtar | 11 + .../checker/testdata/violations_simple.txtar | 12 + internal/checker/testing_test.go | 45 ++++ internal/checker/violation.go | 15 +- internal/checker/violation_test.go | 230 ++++++++++++++++++ 15 files changed, 554 insertions(+), 8 deletions(-) create mode 100644 internal/checker/imports_test.go create mode 100644 internal/checker/testdata/imports_alias.txtar create mode 100644 internal/checker/testdata/imports_all.txtar create mode 100644 internal/checker/testdata/imports_dot.txtar create mode 100644 internal/checker/testdata/imports_many.txtar create mode 100644 internal/checker/testdata/imports_nothing.txtar create mode 100644 internal/checker/testdata/imports_regular.txtar create mode 100644 internal/checker/testdata/violations_new_lines.txtar create mode 100644 internal/checker/testdata/violations_packages.txtar create mode 100644 internal/checker/testdata/violations_simple.txtar create mode 100644 internal/checker/testing_test.go create mode 100644 internal/checker/violation_test.go diff --git a/go.mod b/go.mod index 5d8fab6..48a34e6 100644 --- a/go.mod +++ b/go.mod @@ -2,9 +2,15 @@ module github.com/butuzov/mirror go 1.19 -require golang.org/x/tools v0.8.0 +require ( + github.com/stretchr/testify v1.8.2 + golang.org/x/tools v0.8.0 +) require ( + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect golang.org/x/mod v0.10.0 // indirect golang.org/x/sys v0.7.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/go.sum b/go.sum index d84f5f1..b0dc75b 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,15 @@ +github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= +github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo= +github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= +github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8= +github.com/stretchr/testify v1.8.2/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= golang.org/x/mod v0.10.0 h1:lFO9qtOdlre5W1jxS3r/4szv2/6iXxScdzjoBMXNhYk= golang.org/x/mod v0.10.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o= @@ -5,3 +17,8 @@ golang.org/x/sys v0.7.0 h1:3jlCCIQZPdOYu1h8BkNvLz8Kgwtae2cagcG/VamtZRU= golang.org/x/sys v0.7.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/tools v0.8.0 h1:vSDcovVPld282ceKgDimkRSC8kpaH1dgyc9UMzlt84Y= golang.org/x/tools v0.8.0/go.mod h1:JxBZ99ISMI5ViVkT1tr6tdNmXeTrcpVSD3vZ1RsRdN4= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/checker/imports_test.go b/internal/checker/imports_test.go new file mode 100644 index 0000000..33df643 --- /dev/null +++ b/internal/checker/imports_test.go @@ -0,0 +1,94 @@ +package checker + +import ( + "go/token" + "testing" + + "github.com/stretchr/testify/assert" + "golang.org/x/tools/go/ast/inspector" +) + +func TestImports(t *testing.T) { + testData := []struct { + txtarPath string + importsLen int + hasImports map[string]string + }{ + { + txtarPath: "testdata/imports_nothing.txtar", + importsLen: 0, + }, + { + txtarPath: "testdata/imports_dot.txtar", + importsLen: 1, + hasImports: map[string]string{".": "strings"}, + }, + { + txtarPath: "testdata/imports_alias.txtar", + importsLen: 1, + hasImports: map[string]string{"foo": "strings"}, + }, + { + txtarPath: "testdata/imports_regular.txtar", + importsLen: 1, + hasImports: map[string]string{"strings": "strings"}, + }, + { + txtarPath: "testdata/imports_all.txtar", + importsLen: 22, + hasImports: map[string]string{ + "strings": "strings", + ".": "strings", + "foo1": "strings", + "foo2": "strings", + "foo3": "strings", + "foo4": "strings", + "foo5": "strings", + "foo6": "strings", + "foo7": "strings", + "foo8": "strings", + "foo9": "strings", + "foo10": "strings", + "foo11": "strings", + "foo12": "strings", + "foo13": "strings", + "foo14": "strings", + "foo15": "strings", + "foo16": "strings", + "foo17": "strings", + "foo18": "strings", + "foo19": "strings", + "foo20": "strings", + }, + }, + } + + for _, test := range testData { + test := test + t.Run(test.txtarPath, func(t *testing.T) { + t.Parallel() + + fset := token.NewFileSet() + ar, err := Txtar(t, fset, test.txtarPath) + + assert.Nil(t, err) + assert.Len(t, ar, 1) + + ins := inspector.New(ar) + testImports := Load(fset, ins) + + // assert + assert.Len(t, testImports["a.go"], test.importsLen) + + for k, v := range test.hasImports { + str, ok := testImports.Lookup("a.go", k) + assert.True(t, ok, "Import `%s` not found", k) + assert.Equal(t, v, str, "Wrong package found want(%s) vs got(%s)", v, str) + } + + // test if lookup produce fail + str, ok := testImports.Lookup("a.go", "foobar") + assert.False(t, ok, "found somethig enexpected %s", str) + }) + } +} diff --git a/internal/checker/testdata/imports_alias.txtar b/internal/checker/testdata/imports_alias.txtar new file mode 100644 index 0000000..e2f4761 --- /dev/null +++ b/internal/checker/testdata/imports_alias.txtar @@ -0,0 +1,12 @@ +foobar +-- a.go -- +package testdata + + +import ( + foo "strings" +) + +func M() { + foo.EqualFold("string", "STRING") +} diff --git a/internal/checker/testdata/imports_all.txtar b/internal/checker/testdata/imports_all.txtar new file mode 100644 index 0000000..fae7fbf --- /dev/null +++ b/internal/checker/testdata/imports_all.txtar @@ -0,0 +1,55 @@ +foobar +-- a.go -- +package testdata + + +import ( + foo1 "strings" + foo2 "strings" + foo3 "strings" + foo4 "strings" + foo5 "strings" + foo6 "strings" + foo7 "strings" + foo8 "strings" + foo9 "strings" + foo10 "strings" + foo11 "strings" + foo12 "strings" + foo13 "strings" + foo14 "strings" + foo15 "strings" + foo16 "strings" + foo17 "strings" + foo18 "strings" + foo19 "strings" + foo20 "strings" + . "strings" + "strings" +) + +func M() { + foo1.EqualFold("string", "STRING") + foo2.EqualFold("string", "STRING") + foo3.EqualFold("string", "STRING") + foo4.EqualFold("string", "STRING") + foo5.EqualFold("string", "STRING") + foo6.EqualFold("string", "STRING") + foo7.EqualFold("string", "STRING") + foo8.EqualFold("string", "STRING") + foo9.EqualFold("string", "STRING") + foo10.EqualFold("string", "STRING") + foo11.EqualFold("string", "STRING") + foo12.EqualFold("string", "STRING") + foo13.EqualFold("string", "STRING") + foo14.EqualFold("string", "STRING") + foo15.EqualFold("string", "STRING") + foo16.EqualFold("string", "STRING") + foo17.EqualFold("string", "STRING") + foo18.EqualFold("string", "STRING") + foo19.EqualFold("string", "STRING") + foo20.EqualFold("string", "STRING") + + EqualFold("string", "STRING") + strings.EqualFold("string", "STRING") +} diff --git a/internal/checker/testdata/imports_dot.txtar b/internal/checker/testdata/imports_dot.txtar new file mode 100644 index 0000000..cd13b32 --- /dev/null +++ b/internal/checker/testdata/imports_dot.txtar @@ -0,0 +1,12 @@ +foobar +-- a.go -- +package testdata + + +import ( + . "strings" +) + +func M() { + EqualFold("string", "STRING") +} diff --git a/internal/checker/testdata/imports_many.txtar b/internal/checker/testdata/imports_many.txtar new file mode 100644 index 0000000..6ce6e50 --- /dev/null +++ b/internal/checker/testdata/imports_many.txtar @@ -0,0 +1,22 @@ +foobar +-- a.go -- +package testdata + + +import ( + foo1 "strings" + foo2 "strings" + foo3 "strings" + foo4 "strings" + . "strings" + "strings" +) + +func M() { + foo1.EqualFold("string", "STRING") + foo2.EqualFold("string", "STRING") + foo3.EqualFold("string", "STRING") + foo4.EqualFold("string", "STRING") + EqualFold("string", "STRING") + strings.EqualFold("string", "STRING") +} diff --git a/internal/checker/testdata/imports_nothing.txtar b/internal/checker/testdata/imports_nothing.txtar new file mode 100644 index 0000000..9d82590 --- /dev/null +++ b/internal/checker/testdata/imports_nothing.txtar @@ -0,0 +1,5 @@ +foobar +-- a.go -- +package testdata + +func M() {} diff --git a/internal/checker/testdata/imports_regular.txtar b/internal/checker/testdata/imports_regular.txtar new file mode 100644 index 0000000..99451b4 --- /dev/null +++ b/internal/checker/testdata/imports_regular.txtar @@ -0,0 +1,12 @@ +foobar +-- a.go -- +package testdata + + +import ( + "strings" +) + +func M() { + strings.EqualFold("string", "STRING") +} diff --git a/internal/checker/testdata/violations_new_lines.txtar b/internal/checker/testdata/violations_new_lines.txtar new file mode 100644 index 0000000..97a7853 --- /dev/null +++ b/internal/checker/testdata/violations_new_lines.txtar @@ -0,0 +1,12 @@ +foobar +-- a.go -- +package testdata + + +import ( + "unicode/utf8" +) + +// nothing as fix expected +var foo = utf8.ValidString( + string("foo")) diff --git a/internal/checker/testdata/violations_packages.txtar b/internal/checker/testdata/violations_packages.txtar new file mode 100644 index 0000000..4888a97 --- /dev/null +++ b/internal/checker/testdata/violations_packages.txtar @@ -0,0 +1,11 @@ +foobar +-- a.go -- +package testdata + + +import ( + "unicode/utf8" +) + +// nothing as fix expected +var foo = utf8.ValidString(string("foo")) diff --git a/internal/checker/testdata/violations_simple.txtar b/internal/checker/testdata/violations_simple.txtar new file mode 100644 index 0000000..36b5eab --- /dev/null +++ b/internal/checker/testdata/violations_simple.txtar @@ -0,0 +1,12 @@ +foobar +-- a.go -- +package testdata + + +import ( + "unicode/utf8" +) + +// we are only trying to match selector by description +// and we specially set +var foo = utf8.ValidString(string("foo")) diff --git a/internal/checker/testing_test.go b/internal/checker/testing_test.go new file mode 100644 index 0000000..8f86f16 --- /dev/null +++ b/internal/checker/testing_test.go @@ -0,0 +1,45 @@ +package checker + +import ( + "go/ast" + "go/parser" + "go/token" + "path" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "golang.org/x/tools/txtar" +) + +func ParseExprFrom(t *testing.T, fset *token.FileSet, src any) ast.Expr { + astExpr, err := parser.ParseExprFrom(fset, "a.go", src, parser.AllErrors) + assert.NoError(t, err) + return astExpr +} + +func Txtar(t *testing.T, fset *token.FileSet, txtarPath string) (files []*ast.File, err error) { + t.Helper() + + ar, err := txtar.ParseFile(txtarPath) + if err != nil { + return nil, err + } + + files = make([]*ast.File, 0, len(ar.Files)) + for i := range ar.Files { + file := ar.Files[i] + if !strings.HasSuffix(ar.Files[i].Name, ".go") { + continue + } + + f, err := parser.ParseFile(fset, + path.Base(file.Name), file.Data, parser.AllErrors) + if err != nil { + return nil, err + } + files = append(files, f) + } + + return files, nil +} diff --git a/internal/checker/violation.go b/internal/checker/violation.go index db113a6..217a563 100644 --- a/internal/checker/violation.go +++ b/internal/checker/violation.go @@ -149,23 +149,24 @@ type GolangIssue struct { Original string } -// GolangCI-lint related diagnostic -func (v *Violation) Issue(pass *analysis.Pass) GolangIssue { +// Issue inteanded to be used only with golangci-lint, bu you can use use it +// alongside Diagnostic if you wish. +func (v *Violation) Issue(fSet *token.FileSet) GolangIssue { issue := GolangIssue{ - Start: pass.Fset.Position(v.callExpr.Pos()), - End: pass.Fset.Position(v.callExpr.End()), + Start: fSet.Position(v.callExpr.Pos()), + End: fSet.Position(v.callExpr.End()), Message: v.Message(), } // original expression (useful for debug & requied for replace) var buf bytes.Buffer - printer.Fprint(&buf, pass.Fset, v.callExpr) + printer.Fprint(&buf, fSet, v.callExpr) issue.Original = buf.String() noNl := strings.IndexByte(issue.Original, '\n') < 0 if v.Type == Method && noNl { - fix := v.suggest(pass.Fset) + fix := v.suggest(fSet) issue.InlineFix = string(fix) } @@ -175,7 +176,7 @@ func (v *Violation) Issue(pass *analysis.Pass) GolangIssue { // Hooray! we don't need to change package and redo imports. if v.Type == Function && v.AltPackage == v.Package && noNl { - fix := v.suggest(pass.Fset) + fix := v.suggest(fSet) issue.InlineFix = string(fix) } diff --git a/internal/checker/violation_test.go b/internal/checker/violation_test.go new file mode 100644 index 0000000..810ddf5 --- /dev/null +++ b/internal/checker/violation_test.go @@ -0,0 +1,230 @@ +package checker + +import ( + "go/ast" + "go/importer" + "go/token" + "go/types" + "testing" + + "github.com/stretchr/testify/assert" + "golang.org/x/tools/go/ast/inspector" +) + +func TestViolation(t *testing.T) { + tests := []struct { + Name string + Violation Violation + Expression string + Base string + Args map[int]string + ExpectedSuggest string + Message string + }{ + { + Name: "alt Caller", + Violation: Violation{ + Targets: Strings, + Type: Function, + Package: "regexp", + Caller: "MatchString", + Args: []int{1}, + AltPackage: "foobar", + AltCaller: "Match", + }, + Expression: `regexp.MatchString("[0-9]+", []bytes("foo"))`, + Args: map[int]string{1: `"foo"`}, + Base: "regexp", + ExpectedSuggest: `regexp.Match("[0-9]+", "foo")`, + Message: `avoid allocations with foobar.Match`, + }, + { + Name: "Has More Args Then WeNeed", + Violation: Violation{ + Targets: Strings, + Type: Function, + Package: "regexp", + Caller: "MatchString", + Args: []int{1}, + AltPackage: "foobar", + AltCaller: "Match", + }, + Expression: `regexp.MatchString("[0-9]+", []bytes("foo"))`, + Args: map[int]string{1: `"foo"`, 2: `"foo"`, 3: `"foo"`, 4: `"foo"`}, + Base: "regexp", + ExpectedSuggest: `regexp.Match("[0-9]+", "foo")`, + Message: `avoid allocations with foobar.Match`, + }, + { + Name: "Regular Suggestion Work", + Violation: Violation{ + Targets: Strings, + Type: Function, + Package: "regexp", + Caller: "MatchString", + Args: []int{1}, + AltCaller: "Match", + }, + Expression: `regexp.MatchString("[0-9]+", []bytes("foo"))`, + Args: map[int]string{1: `"foo"`}, + Base: "regexp", + ExpectedSuggest: `regexp.Match("[0-9]+", "foo")`, + Message: `avoid allocations with regexp.Match`, + }, + { + Name: "Methods", + Violation: Violation{ + Targets: Strings, + Type: Method, + Package: "regexp", + Struct: "Regexp", + Caller: "MatchString", + Args: []int{1}, + AltCaller: "Match", + }, + Expression: `re.MatchString("[0-9]+", []bytes("foo"))`, + Args: map[int]string{1: `"foo"`}, + Base: "re", + ExpectedSuggest: `re.Match("[0-9]+", "foo")`, + Message: `avoid allocations with (*regexp.Regexp).Match`, + }, + } + + for i := range tests { + test := tests[i] + t.Run(test.Name, func(t *testing.T) { + fset := token.NewFileSet() + expr := ParseExprFrom(t, fset, test.Expression).(*ast.CallExpr) + args := make(map[int]ast.Expr, len(test.Args)) + base := []byte(test.Base) + + for n := range test.Args { + args[n] = ParseExprFrom(t, fset, test.Args[n]) + } + + v2 := test.Violation.With(base, expr, args) + assert.Equal(t, test.ExpectedSuggest, string(v2.suggest(fset))) + assert.Equal(t, test.Message, v2.Message()) + }) + } +} + +func TestComplex(t *testing.T) { + tests := []struct { + Name string + TxtAr string + ImportPath string + Violation Violation + ExpectedMessage string + ExpectedFix string + }{ + { + Name: "Simple Check", + TxtAr: "testdata/violations_simple.txtar", + ImportPath: "unicode/utf8", + Violation: Violation{ + Type: Function, + Targets: Bytes, + Package: "unicode/utf8", + Caller: "ValidString", + AltCaller: "Valid", + Args: []int{0}, + }, + ExpectedMessage: `avoid allocations with utf8.Valid`, + ExpectedFix: `utf8.Valid("foo")`, + }, + { + Name: "Multiline Fix", + TxtAr: "testdata/violations_new_lines.txtar", + ImportPath: "unicode/utf8", + Violation: Violation{ + Type: Function, + Targets: Bytes, + Package: "unicode/utf8", + Caller: "ValidString", + AltCaller: "Valid", + Args: []int{0}, + }, + ExpectedMessage: `avoid allocations with utf8.Valid`, + ExpectedFix: ``, + }, + { + Name: "Different Packages (No import)", + TxtAr: "testdata/violations_packages.txtar", + ImportPath: "unicode/utf8", + Violation: Violation{ + Type: Function, + Targets: Bytes, + Package: "unicode/utf8", + AltPackage: "unicode/utf8v2", + Caller: "ValidString", + AltCaller: "Valid", + Args: []int{0}, + }, + ExpectedMessage: `avoid allocations with utf8v2.Valid`, + ExpectedFix: ``, + }, + } + + for i := range tests { + test := tests[i] + t.Run(test.Name, func(t *testing.T) { + fset := token.NewFileSet() + ar, err := Txtar(t, fset, test.TxtAr) + assert.Nil(t, err) + + var ( + ins = inspector.New(ar) + conf = types.Config{ + Importer: importer.ForCompiler(fset, "source", nil), + } + info = types.Info{ + Types: make(map[ast.Expr]types.TypeAndValue), + Defs: make(map[*ast.Ident]types.Object), + Uses: make(map[*ast.Ident]types.Object), + } + ) + + // ------ Setup ---------------------------------------------------------- + + _, err = conf.Check("source", fset, ar, &info) + assert.NoError(t, err) + + check := New([]Violation{test.Violation}) + check.Type = WrapType(&info) + check.Print = WrapPrint(fset) + + var happend bool + + ins.Preorder([]ast.Node{(*ast.CallExpr)(nil)}, func(n ast.Node) { + // allow to check only first call + if happend { + return + } + happend = true + // ---- test -------------------------------------------------- + callExpr := n.(*ast.CallExpr) + expr := callExpr.Fun.(*ast.SelectorExpr) + x, ok := expr.X.(*ast.Ident) + assert.True(t, ok) + + name := expr.Sel.Name + // skipping import checks with correct import path + v := check.Match(test.ImportPath, name) + assert.NotNil(t, v) + assert.Equal(t, *v, test.Violation) + + args, found := check.Handle(v, callExpr) + assert.True(t, found, "no string to string conversions found") + v2 := v.With(check.Print(x), callExpr, args) + + gciIssue := v2.Issue(fset) + + assert.Equal(t, test.ExpectedFix, gciIssue.InlineFix, "fix not match") + assert.Equal(t, test.ExpectedMessage, gciIssue.Message, "message not match") + }) + + assert.True(t, happend, "Test Not Happend") + }) + } +}