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 <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
diff --git a/internal/lsp/cmd/cmd_test.go b/internal/lsp/cmd/cmd_test.go
index af04e8d..0e417c6 100644
--- a/internal/lsp/cmd/cmd_test.go
+++ b/internal/lsp/cmd/cmd_test.go
@@ -17,7 +17,6 @@
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 @@
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 98d390c..024d987 100644
--- a/internal/lsp/cmd/test/check.go
+++ b/internal/lsp/cmd/test/check.go
@@ -10,10 +10,8 @@
"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 @@
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 @@
}
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 3daf9b6..7c8e21b 100644
--- a/internal/lsp/cmd/test/cmdtest.go
+++ b/internal/lsp/cmd/test/cmdtest.go
@@ -8,6 +8,7 @@
import (
"bytes"
"context"
+ "fmt"
"io/ioutil"
"os"
"path/filepath"
@@ -16,25 +17,54 @@
"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 @@
//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 40da50b..9cd1415 100644
--- a/internal/lsp/cmd/test/definition.go
+++ b/internal/lsp/cmd/test/definition.go
@@ -10,10 +10,8 @@
"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 @@
}
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 @@
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 30f977e..6edb58e 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 355f0d9..f5b4631 100644
--- a/internal/lsp/cmd/test/format.go
+++ b/internal/lsp/cmd/test/format.go
@@ -13,10 +13,8 @@
"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 @@
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 c51897a..c5b1a50 100644
--- a/internal/lsp/cmd/test/imports.go
+++ b/internal/lsp/cmd/test/imports.go
@@ -8,19 +8,13 @@
"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 c74ea6a..88df768 100644
--- a/internal/lsp/cmd/test/links.go
+++ b/internal/lsp/cmd/test/links.go
@@ -8,11 +8,9 @@
"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 @@
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 9cb5a66..3039f53 100644
--- a/internal/lsp/cmd/test/references.go
+++ b/internal/lsp/cmd/test/references.go
@@ -9,9 +9,6 @@
"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 @@
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 9b227b9..0fe2d1e 100644
--- a/internal/lsp/cmd/test/rename.go
+++ b/internal/lsp/cmd/test/rename.go
@@ -8,33 +8,22 @@
"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 5ba55bd..8f55aea 100644
--- a/internal/lsp/cmd/test/signature.go
+++ b/internal/lsp/cmd/test/signature.go
@@ -8,9 +8,7 @@
"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 @@
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 d41c590..f88131b 100644
--- a/internal/lsp/cmd/test/suggested_fix.go
+++ b/internal/lsp/cmd/test/suggested_fix.go
@@ -7,19 +7,13 @@
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 05f00ab..055be03 100644
--- a/internal/lsp/cmd/test/symbols.go
+++ b/internal/lsp/cmd/test/symbols.go
@@ -7,23 +7,13 @@
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
}))
