From 0ae87fff1b85673ca22fae0603daf76d984734ac Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Thu, 21 Nov 2019 19:58:38 -0500 Subject: [PATCH] internal/lsp: fix a race in the command line tests except the race was a symptom of a larger problem, so the fix actually invovles cleaning up the way we run command line tests totally to have common shared infrastructure, and also to clean up the way we handle errors and paths into the temporary directory Fixes: golang/go#35436 Change-Id: I4c5602607bb70e082056132baa3d4b0f8df6b13b Reviewed-on: https://go-review.googlesource.com/c/tools/+/208269 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/cmd/cmd_test.go | 6 +- internal/lsp/cmd/test/check.go | 11 +- internal/lsp/cmd/test/cmdtest.go | 148 ++++++++++++++++++------- internal/lsp/cmd/test/definition.go | 10 +- internal/lsp/cmd/test/folding_range.go | 13 +-- internal/lsp/cmd/test/format.go | 14 +-- internal/lsp/cmd/test/imports.go | 8 +- internal/lsp/cmd/test/links.go | 8 +- internal/lsp/cmd/test/references.go | 15 +-- internal/lsp/cmd/test/rename.go | 19 +--- internal/lsp/cmd/test/signature.go | 9 +- internal/lsp/cmd/test/suggested_fix.go | 8 +- internal/lsp/cmd/test/symbols.go | 12 +- 13 files changed, 129 insertions(+), 152 deletions(-) diff --git a/internal/lsp/cmd/cmd_test.go b/internal/lsp/cmd/cmd_test.go index af04e8dc5a..0e417c68a9 100644 --- a/internal/lsp/cmd/cmd_test.go +++ b/internal/lsp/cmd/cmd_test.go @@ -17,7 +17,6 @@ import ( cmdtest "golang.org/x/tools/internal/lsp/cmd/test" "golang.org/x/tools/internal/lsp/tests" "golang.org/x/tools/internal/testenv" - "golang.org/x/tools/internal/tool" ) func TestMain(m *testing.M) { @@ -53,9 +52,8 @@ func TestDefinitionHelpExample(t *testing.T) { fmt.Sprintf("%v:%v:%v", thisFile, cmd.ExampleLine, cmd.ExampleColumn), fmt.Sprintf("%v:#%v", thisFile, cmd.ExampleOffset)} { args := append(baseArgs, query) - got := cmdtest.CaptureStdOut(t, func() { - _ = tool.Run(tests.Context(t), cmd.New("gopls-test", "", nil, nil), args) - }) + r := cmdtest.NewRunner(nil, nil, tests.Context(t), nil) + got, _ := r.NormalizeGoplsCmd(t, args...) if !expect.MatchString(got) { t.Errorf("test with %v\nexpected:\n%s\ngot:\n%s", args, expect, got) } diff --git a/internal/lsp/cmd/test/check.go b/internal/lsp/cmd/test/check.go index 98d390c253..024d987818 100644 --- a/internal/lsp/cmd/test/check.go +++ b/internal/lsp/cmd/test/check.go @@ -10,10 +10,8 @@ import ( "strings" "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/tool" ) func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnostic) { @@ -21,11 +19,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti return } fname := uri.Filename() - args := []string{"-remote=internal", "check", fname} - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options) - out := CaptureStdOut(t, func() { - _ = tool.Run(r.ctx, app, args) - }) + out, _ := r.RunGoplsCmd(t, "check", fname) // parse got into a collection of reports got := map[string]struct{}{} for _, l := range strings.Split(out, "\n") { @@ -48,13 +42,14 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []source.Diagnosti } l = fmt.Sprintf("%s: %s", s, strings.TrimSpace(bits[1])) } - got[l] = struct{}{} + got[r.NormalizePrefix(l)] = struct{}{} } for _, diag := range want { expect := fmt.Sprintf("%v:%v:%v: %v", uri.Filename(), diag.Range.Start.Line+1, diag.Range.Start.Character+1, diag.Message) if diag.Range.Start.Character == 0 { expect = fmt.Sprintf("%v:%v: %v", uri.Filename(), diag.Range.Start.Line+1, diag.Message) } + expect = r.NormalizePrefix(expect) // Skip the badimport test for now, until we do a better job with diagnostic ranges. if strings.Contains(uri.Filename(), "badimport") { continue diff --git a/internal/lsp/cmd/test/cmdtest.go b/internal/lsp/cmd/test/cmdtest.go index 3daf9b620c..7c8e21bdf7 100644 --- a/internal/lsp/cmd/test/cmdtest.go +++ b/internal/lsp/cmd/test/cmdtest.go @@ -8,6 +8,7 @@ package cmdtest import ( "bytes" "context" + "fmt" "io/ioutil" "os" "path/filepath" @@ -16,25 +17,54 @@ import ( "testing" "golang.org/x/tools/go/packages/packagestest" + "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/lsp/tests" "golang.org/x/tools/internal/span" + "golang.org/x/tools/internal/tool" ) type runner struct { - exporter packagestest.Exporter - data *tests.Data - ctx context.Context - options func(*source.Options) + exporter packagestest.Exporter + data *tests.Data + ctx context.Context + options func(*source.Options) + normalizers []normalizer } -func NewRunner(exporter packagestest.Exporter, data *tests.Data, ctx context.Context, options func(*source.Options)) tests.Tests { - return &runner{ - exporter: exporter, - data: data, - ctx: ctx, - options: options, +type normalizer struct { + path string + slashed string + escaped string + fragment string +} + +func NewRunner(exporter packagestest.Exporter, data *tests.Data, ctx context.Context, options func(*source.Options)) *runner { + r := &runner{ + exporter: exporter, + data: data, + ctx: ctx, + options: options, + normalizers: make([]normalizer, 0, len(data.Exported.Modules)), } + // build the path normalizing patterns + for _, m := range data.Exported.Modules { + for fragment := range m.Files { + n := normalizer{ + path: data.Exported.File(m.Name, fragment), + fragment: fragment, + } + if n.slashed = filepath.ToSlash(n.path); n.slashed == n.path { + n.slashed = "" + } + quoted := strconv.Quote(n.path) + if n.escaped = quoted[1 : len(quoted)-1]; n.escaped == n.path { + n.escaped = "" + } + r.normalizers = append(r.normalizers, n) + } + } + return r } func (r *runner) Completion(t *testing.T, src span.Span, test tests.Completion, items tests.CompletionItems) { @@ -77,56 +107,92 @@ func (r *runner) Implementation(t *testing.T, spn span.Span, imp tests.Implement //TODO: add implements tests when it works } -func CaptureStdOut(t testing.TB, f func()) string { - r, out, err := os.Pipe() +func (r *runner) RunGoplsCmd(t testing.TB, args ...string) (string, string) { + rStdout, wStdout, err := os.Pipe() if err != nil { t.Fatal(err) } - old := os.Stdout + oldStdout := os.Stdout + rStderr, wStderr, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + oldStderr := os.Stderr defer func() { - os.Stdout = old - out.Close() - r.Close() + os.Stdout = oldStdout + os.Stderr = oldStderr + wStdout.Close() + rStdout.Close() + wStderr.Close() + rStderr.Close() }() - os.Stdout = out - f() - out.Close() - data, err := ioutil.ReadAll(r) + os.Stdout = wStdout + os.Stderr = wStderr + err = tool.Run(tests.Context(t), + cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options), + append([]string{"-remote=internal"}, args...)) + if err != nil { + fmt.Fprint(os.Stderr, err) + } + wStdout.Close() + wStderr.Close() + stdout, err := ioutil.ReadAll(rStdout) if err != nil { t.Fatal(err) } - return string(data) + stderr, err := ioutil.ReadAll(rStderr) + if err != nil { + t.Fatal(err) + } + return string(stdout), string(stderr) } -// normalizePaths replaces all paths present in s with just the fragment portion +func (r *runner) NormalizeGoplsCmd(t testing.TB, args ...string) (string, string) { + stdout, stderr := r.RunGoplsCmd(t, args...) + return r.Normalize(stdout), r.Normalize(stderr) +} + +// NormalizePrefix normalizes a single path at the front of the input string. +func (r *runner) NormalizePrefix(s string) string { + for _, n := range r.normalizers { + if t := strings.TrimPrefix(s, n.path); t != s { + return n.fragment + t + } + if t := strings.TrimPrefix(s, n.slashed); t != s { + return n.fragment + t + } + if t := strings.TrimPrefix(s, n.escaped); t != s { + return n.fragment + t + } + } + return s +} + +// Normalize replaces all paths present in s with just the fragment portion // this is used to make golden files not depend on the temporary paths of the files -func normalizePaths(data *tests.Data, s string) string { +func (r *runner) Normalize(s string) string { type entry struct { path string index int fragment string } - match := make([]entry, 0, len(data.Exported.Modules)) + match := make([]entry, 0, len(r.normalizers)) // collect the initial state of all the matchers - for _, m := range data.Exported.Modules { - for fragment := range m.Files { - filename := data.Exported.File(m.Name, fragment) - index := strings.Index(s, filename) + for _, n := range r.normalizers { + index := strings.Index(s, n.path) + if index >= 0 { + match = append(match, entry{n.path, index, n.fragment}) + } + if n.slashed != "" { + index := strings.Index(s, n.slashed) if index >= 0 { - match = append(match, entry{filename, index, fragment}) + match = append(match, entry{n.slashed, index, n.fragment}) } - if slash := filepath.ToSlash(filename); slash != filename { - index := strings.Index(s, slash) - if index >= 0 { - match = append(match, entry{slash, index, fragment}) - } - } - quoted := strconv.Quote(filename) - if escaped := quoted[1 : len(quoted)-1]; escaped != filename { - index := strings.Index(s, escaped) - if index >= 0 { - match = append(match, entry{escaped, index, fragment}) - } + } + if n.escaped != "" { + index := strings.Index(s, n.escaped) + if index >= 0 { + match = append(match, entry{n.escaped, index, n.fragment}) } } } diff --git a/internal/lsp/cmd/test/definition.go b/internal/lsp/cmd/test/definition.go index 40da50be61..9cd1415de5 100644 --- a/internal/lsp/cmd/test/definition.go +++ b/internal/lsp/cmd/test/definition.go @@ -10,10 +10,8 @@ import ( "strings" "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/lsp/tests" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/tool" ) const ( @@ -40,7 +38,7 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) { } d.Src = span.New(d.Src.URI(), span.NewPoint(0, 0, d.Src.Start().Offset()), span.Point{}) for _, mode := range godefModes { - args := []string{"-remote=internal", "query"} + args := []string{"query"} tag := d.Name + "-definition" if mode&jsonGoDef != 0 { tag += "-json" @@ -49,11 +47,7 @@ func (r *runner) Definition(t *testing.T, spn span.Span, d tests.Definition) { args = append(args, "definition") uri := d.Src.URI() args = append(args, fmt.Sprint(d.Src)) - got := CaptureStdOut(t, func() { - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options) - _ = tool.Run(r.ctx, app, args) - }) - got = normalizePaths(r.data, got) + got, _ := r.NormalizeGoplsCmd(t, args...) if mode&jsonGoDef != 0 && runtime.GOOS == "windows" { got = strings.Replace(got, "file:///", "file://", -1) } diff --git a/internal/lsp/cmd/test/folding_range.go b/internal/lsp/cmd/test/folding_range.go index 30f977ed3c..6edb58ee0c 100644 --- a/internal/lsp/cmd/test/folding_range.go +++ b/internal/lsp/cmd/test/folding_range.go @@ -1,27 +1,16 @@ package cmdtest import ( - "fmt" "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/tool" ) func (r *runner) FoldingRanges(t *testing.T, spn span.Span) { goldenTag := "foldingRange-cmd" uri := spn.URI() filename := uri.Filename() - - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env, r.options) - got := CaptureStdOut(t, func() { - err := tool.Run(r.ctx, app, append([]string{"-remote=internal", "folding_ranges"}, filename)) - if err != nil { - fmt.Println(err) - } - }) - + got, _ := r.NormalizeGoplsCmd(t, "folding_ranges", filename) expect := string(r.data.Golden(goldenTag, filename, func() ([]byte, error) { return []byte(got), nil })) diff --git a/internal/lsp/cmd/test/format.go b/internal/lsp/cmd/test/format.go index 355f0d9cf7..f5b4631604 100644 --- a/internal/lsp/cmd/test/format.go +++ b/internal/lsp/cmd/test/format.go @@ -13,10 +13,8 @@ import ( "strings" "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/span" "golang.org/x/tools/internal/testenv" - "golang.org/x/tools/internal/tool" ) func (r *runner) Format(t *testing.T, spn span.Span) { @@ -26,25 +24,19 @@ func (r *runner) Format(t *testing.T, spn span.Span) { expect := string(r.data.Golden(tag, filename, func() ([]byte, error) { cmd := exec.Command("gofmt", filename) contents, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files - contents = []byte(normalizePaths(r.data, fixFileHeader(string(contents)))) + contents = []byte(r.Normalize(fixFileHeader(string(contents)))) return contents, nil })) if expect == "" { //TODO: our error handling differs, for now just skip unformattable files t.Skip("Unformattable file") } - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env, r.options) - got := CaptureStdOut(t, func() { - _ = tool.Run(r.ctx, app, append([]string{"-remote=internal", "format"}, filename)) - }) - got = normalizePaths(r.data, got) + got, _ := r.NormalizeGoplsCmd(t, "format", filename) if expect != got { t.Errorf("format failed for %s expected:\n%s\ngot:\n%s", filename, expect, got) } // now check we can build a valid unified diff - unified := CaptureStdOut(t, func() { - _ = tool.Run(r.ctx, app, append([]string{"-remote=internal", "format", "-d"}, filename)) - }) + unified, _ := r.NormalizeGoplsCmd(t, "format", "-d", filename) checkUnified(t, filename, expect, unified) } diff --git a/internal/lsp/cmd/test/imports.go b/internal/lsp/cmd/test/imports.go index c51897a51a..c5b1a50715 100644 --- a/internal/lsp/cmd/test/imports.go +++ b/internal/lsp/cmd/test/imports.go @@ -8,19 +8,13 @@ import ( "os/exec" "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/tool" ) func (r *runner) Import(t *testing.T, spn span.Span) { uri := spn.URI() filename := uri.Filename() - args := []string{"-remote=internal", "imports", filename} - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options) - got := CaptureStdOut(t, func() { - _ = tool.Run(r.ctx, app, args) - }) + got, _ := r.NormalizeGoplsCmd(t, "imports", filename) want := string(r.data.Golden("goimports", filename, func() ([]byte, error) { cmd := exec.Command("goimports", filename) out, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files diff --git a/internal/lsp/cmd/test/links.go b/internal/lsp/cmd/test/links.go index c74ea6a2c6..88df768323 100644 --- a/internal/lsp/cmd/test/links.go +++ b/internal/lsp/cmd/test/links.go @@ -8,11 +8,9 @@ import ( "encoding/json" "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/tests" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/tool" ) func (r *runner) Link(t *testing.T, uri span.URI, wantLinks []tests.Link) { @@ -20,11 +18,7 @@ func (r *runner) Link(t *testing.T, uri span.URI, wantLinks []tests.Link) { if err != nil { t.Fatal(err) } - args := []string{"-remote=internal", "links", "-json", uri.Filename()} - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options) - out := CaptureStdOut(t, func() { - _ = tool.Run(r.ctx, app, args) - }) + out, _ := r.NormalizeGoplsCmd(t, "links", "-json", uri.Filename()) var got []protocol.DocumentLink err = json.Unmarshal([]byte(out), &got) if err != nil { diff --git a/internal/lsp/cmd/test/references.go b/internal/lsp/cmd/test/references.go index 9cb5a66982..3039f53483 100644 --- a/internal/lsp/cmd/test/references.go +++ b/internal/lsp/cmd/test/references.go @@ -9,9 +9,6 @@ import ( "sort" "testing" - "golang.org/x/tools/internal/lsp/cmd" - "golang.org/x/tools/internal/tool" - "golang.org/x/tools/internal/span" ) @@ -25,20 +22,12 @@ func (r *runner) References(t *testing.T, spn span.Span, itemList []span.Span) { for _, i := range itemStrings { expect += i + "\n" } + expect = r.Normalize(expect) uri := spn.URI() filename := uri.Filename() target := filename + fmt.Sprintf(":%v:%v", spn.Start().Line(), spn.Start().Column()) - - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env, r.options) - got := CaptureStdOut(t, func() { - err := tool.Run(r.ctx, app, append([]string{"-remote=internal", "references"}, target)) - if err != nil { - fmt.Println(spn.Start().Line()) - fmt.Println(err) - } - }) - + got, _ := r.NormalizeGoplsCmd(t, "references", target) if expect != got { t.Errorf("references failed for %s expected:\n%s\ngot:\n%s", target, expect, got) } diff --git a/internal/lsp/cmd/test/rename.go b/internal/lsp/cmd/test/rename.go index 9b227b9bd9..0fe2d1e182 100644 --- a/internal/lsp/cmd/test/rename.go +++ b/internal/lsp/cmd/test/rename.go @@ -8,33 +8,22 @@ import ( "fmt" "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/tool" ) func (r *runner) Rename(t *testing.T, spn span.Span, newText string) { filename := spn.URI().Filename() goldenTag := newText + "-rename" - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env, r.options) loc := fmt.Sprintf("%v", spn) - var err error - got := CaptureStdOut(t, func() { - err = tool.Run(r.ctx, app, []string{"-remote=internal", "rename", loc, newText}) - }) - if err != nil { - got = err.Error() - } - got = normalizePaths(r.data, got) + got, err := r.NormalizeGoplsCmd(t, "rename", loc, newText) + got += err expect := string(r.data.Golden(goldenTag, filename, func() ([]byte, error) { return []byte(got), nil })) if expect != got { - t.Errorf("rename failed with %v %v expected:\n%s\ngot:\n%s", loc, newText, expect, got) + t.Errorf("rename failed with %v %v\nexpected:\n%s\ngot:\n%s", loc, newText, expect, got) } // now check we can build a valid unified diff - unified := CaptureStdOut(t, func() { - _ = tool.Run(r.ctx, app, []string{"-remote=internal", "rename", "-d", loc, newText}) - }) + unified, _ := r.NormalizeGoplsCmd(t, "rename", "-d", loc, newText) checkUnified(t, filename, expect, unified) } diff --git a/internal/lsp/cmd/test/signature.go b/internal/lsp/cmd/test/signature.go index 5ba55bd86d..8f55aeaa35 100644 --- a/internal/lsp/cmd/test/signature.go +++ b/internal/lsp/cmd/test/signature.go @@ -8,9 +8,7 @@ import ( "fmt" "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/lsp/source" - "golang.org/x/tools/internal/tool" "golang.org/x/tools/internal/span" ) @@ -23,12 +21,7 @@ func (r *runner) SignatureHelp(t *testing.T, spn span.Span, expectedSignature *s uri := spn.URI() filename := uri.Filename() target := filename + fmt.Sprintf(":%v:%v", spn.Start().Line(), spn.Start().Column()) - - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env, r.options) - got := CaptureStdOut(t, func() { - tool.Run(r.ctx, app, append([]string{"-remote=internal", "signature"}, target)) - }) - + got, _ := r.NormalizeGoplsCmd(t, "signature", target) expect := string(r.data.Golden(goldenTag, filename, func() ([]byte, error) { return []byte(got), nil })) diff --git a/internal/lsp/cmd/test/suggested_fix.go b/internal/lsp/cmd/test/suggested_fix.go index d41c590129..f88131b92a 100644 --- a/internal/lsp/cmd/test/suggested_fix.go +++ b/internal/lsp/cmd/test/suggested_fix.go @@ -7,19 +7,13 @@ package cmdtest import ( "testing" - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/tool" ) func (r *runner) SuggestedFix(t *testing.T, spn span.Span) { uri := spn.URI() filename := uri.Filename() - args := []string{"-remote=internal", "fix", "-a", filename} - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Exported.Config.Env, r.options) - got := CaptureStdOut(t, func() { - _ = tool.Run(r.ctx, app, args) - }) + got, _ := r.NormalizeGoplsCmd(t, "fix", "-a", filename) want := string(r.data.Golden("suggestedfix", filename, func() ([]byte, error) { return []byte(got), nil })) diff --git a/internal/lsp/cmd/test/symbols.go b/internal/lsp/cmd/test/symbols.go index 05f00abc59..055be03082 100644 --- a/internal/lsp/cmd/test/symbols.go +++ b/internal/lsp/cmd/test/symbols.go @@ -7,23 +7,13 @@ package cmdtest import ( "testing" - "fmt" - - "golang.org/x/tools/internal/lsp/cmd" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/span" - "golang.org/x/tools/internal/tool" ) func (r *runner) Symbols(t *testing.T, uri span.URI, expectedSymbols []protocol.DocumentSymbol) { filename := uri.Filename() - app := cmd.New("gopls-test", r.data.Config.Dir, r.data.Config.Env, r.options) - got := CaptureStdOut(t, func() { - err := tool.Run(r.ctx, app, append([]string{"-remote=internal", "symbols"}, filename)) - if err != nil { - fmt.Println(err) - } - }) + got, _ := r.NormalizeGoplsCmd(t, "symbols", filename) expect := string(r.data.Golden("symbols", filename, func() ([]byte, error) { return []byte(got), nil }))