internal/lsp: switch golden files to use txtar

I was about to add some more tests and it caused a huge number of golden files,
which was hard to deal with. Now all the golden files are packed into a single
.golden archive in the txtar format.
I also changed the tagging key for hover results to use the marker name rather
than the line and column, as it makes it more stable against test data changes.

Change-Id: Iaa1f54ab55a41d380db67b9f6f928fa7a52d9a5e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/174877
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/cmd/format_test.go

@@ -5,9 +5,7 @@
 package cmd_test
 
 import (
-	"bytes"
 	"context"
-	"io/ioutil"
 	"os/exec"
 	"regexp"
 	"strings"
@@ -33,13 +31,11 @@ func (r *runner) Format(t *testing.T, data tests.Formats) {
 				t.Fatal(err)
 			}
 			args := append(mode, filename)
-			expect := string(r.data.Golden(tag, filename, func(golden string) error {
+			expect := string(r.data.Golden(tag, filename, func() ([]byte, error) {
 				cmd := exec.Command("gofmt", args...)
-				buf := &bytes.Buffer{}
+				contents, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files
-				cmd.Stdout = buf
+				contents = []byte(r.normalizePaths(fixFileHeader(string(contents))))
-				cmd.Run() // ignore error, sometimes we have intentionally ungofmt-able files
+				return contents, nil
-				contents := r.normalizePaths(fixFileHeader(buf.String()))
-				return ioutil.WriteFile(golden, []byte(contents), 0666)
 			}))
 			if expect == "" {
 				//TODO: our error handling differs, for now just skip unformattable files

internal/lsp/lsp_test.go

@@ -9,8 +9,6 @@ import (
 	"context"
 	"fmt"
 	"go/token"
-	"io/ioutil"
-	"os"
 	"os/exec"
 	"sort"
 	"strings"
@@ -290,16 +288,10 @@ func (r *runner) Format(t *testing.T, data tests.Formats) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		gofmted := string(r.data.Golden("gofmt", filename, func(golden string) error {
+		gofmted := string(r.data.Golden("gofmt", filename, func() ([]byte, error) {
 			cmd := exec.Command("gofmt", filename)
-			stdout, err := os.Create(golden)
+			out, _ := cmd.Output() // ignore error, sometimes we have intentionally ungofmt-able files
-			if err != nil {
+			return out, nil
-				return err
-			}
-			defer stdout.Close()
-			cmd.Stdout = stdout
-			cmd.Run() // ignore error, sometimes we have intentionally ungofmt-able files
-			return nil
 		}))
 
 		edits, err := r.server.Formatting(context.Background(), &protocol.DocumentFormattingParams{
@@ -365,13 +357,13 @@ func (r *runner) Definition(t *testing.T, data tests.Definitions) {
 			t.Errorf("for %v got %v want %v", d.Src, def, d.Def)
 		}
 		if hover != nil {
-			tag := fmt.Sprintf("hover-%d-%d", d.Def.Start().Line(), d.Def.Start().Column())
+			tag := fmt.Sprintf("%s-hover", d.Name)
-			filename, err := d.Def.URI().Filename()
+			filename, err := d.Src.URI().Filename()
 			if err != nil {
 				t.Fatalf("failed for %v: %v", d.Def, err)
 			}
-			expectHover := string(r.data.Golden(tag, filename, func(golden string) error {
+			expectHover := string(r.data.Golden(tag, filename, func() ([]byte, error) {
-				return ioutil.WriteFile(golden, []byte(hover.Contents.Value), 0666)
+				return []byte(hover.Contents.Value), nil
 			}))
 			if hover.Contents.Value != expectHover {
 				t.Errorf("for %v got %q want %q", d.Src, hover.Contents.Value, expectHover)

internal/lsp/reset_golden.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+
+find ./internal/lsp/ -name *.golden -delete
+go test ./internal/lsp/ ./internal/lsp/cmd -golden

internal/lsp/testdata/format/bad_format.go.golden

@@ -1,3 +1,25 @@
+-- gofmt --
+package format //@format("package")
+
+import (
+	"fmt"
+	"log"
+	"runtime"
+)
+
+func hello() {
+
+	var x int //@diag("x", "LSP", "x declared but not used")
+}
+
+func hi() {
+	runtime.GOROOT()
+	fmt.Printf("")
+
+	log.Printf("")
+}
+
+-- gofmt-d --
 --- format/bad_format.go.orig
 +++ format/bad_format.go
 @@ -1,16 +1,13 @@
@@ -18,3 +40,4 @@
  	var x int //@diag("x", "LSP", "x declared but not used")
  }
  
+

internal/lsp/testdata/format/bad_format.gofmt.golden.go

@@ -1,19 +0,0 @@
-package format //@format("package")
-
-import (
-	"fmt"
-	"log"
-	"runtime"
-)
-
-func hello() {
-
-	var x int //@diag("x", "LSP", "x declared but not used")
-}
-
-func hi() {
-	runtime.GOROOT()
-	fmt.Printf("")
-
-	log.Printf("")
-}

internal/lsp/testdata/format/good_format.go.golden

@@ -1,3 +1,4 @@
+-- gofmt --
 package format //@format("package")
 
 import (
@@ -7,3 +8,6 @@ import (
 func goodbye() {
 	log.Printf("byeeeee")
 }
+
+-- gofmt-d --
+

internal/lsp/testdata/format/newline_format.go.golden

@@ -1,3 +1,8 @@
+-- gofmt --
+package format //@format("package")
+func _()       {}
+
+-- gofmt-d --
 --- format/newline_format.go.orig
 +++ format/newline_format.go
 @@ -1,2 +1,2 @@
@@ -5,3 +10,4 @@
 -func _() {}
 \ No newline at end of file
 +func _()       {}
+

internal/lsp/testdata/format/newline_format.gofmt.golden.go

@@ -1,2 +0,0 @@
-package format //@format("package")
-func _()       {}

internal/lsp/testdata/format/one_line.go.golden

@@ -1,6 +1,11 @@
+-- gofmt --
+package format //@format("package")
+
+-- gofmt-d --
 --- format/one_line.go.orig
 +++ format/one_line.go
 @@ -1 +1 @@
 -package format //@format("package")
 \ No newline at end of file
 +package format //@format("package")
+

internal/lsp/testdata/format/one_line.gofmt.golden.go

@@ -1 +0,0 @@
-package format //@format("package")

internal/lsp/testdata/godef/a/a.go.golden

@@ -0,0 +1,6 @@
+-- Random-hover --
+func Random() int
+-- Random2-hover --
+func Random2(y int) int
+-- err-hover --
+var err error

internal/lsp/testdata/godef/a/a.hover-14-6.golden.go

@@ -1 +0,0 @@
-var err error

internal/lsp/testdata/godef/a/a.hover-7-6.golden.go

@@ -1 +0,0 @@
-type a.A string

internal/lsp/testdata/godef/a/a.hover-9-6.golden.go

@@ -1 +0,0 @@
-func a.Stuff()

internal/lsp/testdata/godef/a/random.go.golden

@@ -0,0 +1,6 @@
+-- PosSum-hover --
+func (*Pos).Sum() int
+-- PosX-hover --
+field x int
+-- RandomParamY-hover --
+var y int

internal/lsp/testdata/godef/a/random.hover-13-2.golden.go

@@ -1 +0,0 @@
-field x int

internal/lsp/testdata/godef/a/random.hover-16-15.golden.go

@@ -1 +0,0 @@
-func (*Pos).Sum() int

internal/lsp/testdata/godef/a/random.hover-3-6.golden.go

@@ -1 +0,0 @@
-func Random() int

internal/lsp/testdata/godef/a/random.hover-8-14.golden.go

@@ -1 +0,0 @@
-var y int

internal/lsp/testdata/godef/a/random.hover-8-6.golden.go

@@ -1 +0,0 @@
-func Random2(y int) int

internal/lsp/testdata/godef/b/b.go.golden

@@ -0,0 +1,16 @@
+-- A-hover --
+type a.A string
+-- S1-hover --
+type S1 struct{F1 int; S2; a.A}
+-- S1F1-hover --
+field F1 int
+-- S1S2-hover --
+field S2 S2
+-- S2-hover --
+type S2 struct{F1 string; F2 int; *a.A}
+-- S2F1-hover --
+field F1 string
+-- S2F2-hover --
+field F2 int
+-- Stuff-hover --
+func a.Stuff()

internal/lsp/testdata/godef/b/b.hover-11-6.golden.go

@@ -1 +0,0 @@
-type S2 struct{F1 string; F2 int; *a.A}

internal/lsp/testdata/godef/b/b.hover-12-2.golden.go

@@ -1 +0,0 @@
-field F1 string

internal/lsp/testdata/godef/b/b.hover-13-2.golden.go

@@ -1 +0,0 @@
-field F2 int

internal/lsp/testdata/godef/b/b.hover-5-6.golden.go

@@ -1 +0,0 @@
-type S1 struct{F1 int; S2; a.A}

internal/lsp/testdata/godef/b/b.hover-6-2.golden.go

@@ -1 +0,0 @@
-field F1 int

internal/lsp/testdata/godef/b/b.hover-7-2.golden.go

@@ -1 +0,0 @@
-field S2 S2

internal/lsp/testdata/godef/b/c.go.golden

@@ -0,0 +1,4 @@
+-- S1-hover --
+type S1 struct{F1 int; S2; a.A}
+-- S1F1-hover --
+field F1 int

internal/lsp/testdata/godef/broken/unclosedIf.go.golden

@@ -0,0 +1,2 @@
+-- myUnclosedIf-hover --
+var myUnclosedIf string

internal/lsp/testdata/godef/broken/unclosedIf.hover-7-7.golden.go

@@ -1 +0,0 @@
-var myUnclosedIf string

internal/lsp/testdata/noparse_format/noparse_format.go.golden

@@ -0,0 +1,4 @@
+-- gofmt --
+
+-- gofmt-d --
+

internal/lsp/tests/tests.go

@@ -12,9 +12,9 @@ import (
 	"go/token"
 	"io/ioutil"
 	"os/exec"
-	"path"
 	"path/filepath"
 	"runtime"
+	"sort"
 	"strings"
 	"testing"
 
@@ -22,6 +22,7 @@ import (
 	"golang.org/x/tools/go/packages/packagestest"
 	"golang.org/x/tools/internal/lsp/source"
 	"golang.org/x/tools/internal/span"
+	"golang.org/x/tools/internal/txtar"
 )
 
 // We hardcode the expected number of test cases to ensure that all tests
@@ -40,10 +41,10 @@ const (
 )
 
 const (
-	overlayFile = ".overlay"
+	overlayFileSuffix = ".overlay"
-	goldenFile  = ".golden"
+	goldenFileSuffix  = ".golden"
-	inFile      = ".in"
+	inFileSuffix      = ".in"
-	testModule  = "golang.org/x/tools/internal/lsp"
+	testModule        = "golang.org/x/tools/internal/lsp"
 )
 
 var updateGolden = flag.Bool("golden", false, "Update golden files")
@@ -78,6 +79,7 @@ type Data struct {
 	t         testing.TB
 	fragments map[string]string
 	dir       string
+	golden    map[string]*Golden
 }
 
 type Tests interface {
@@ -92,6 +94,7 @@ type Tests interface {
 }
 
 type Definition struct {
+	Name   string
 	Src    span.Span
 	IsType bool
 	Flags  string
@@ -110,6 +113,12 @@ type Link struct {
 	Target string
 }
 
+type Golden struct {
+	Filename string
+	Archive  *txtar.Archive
+	Modified bool
+}
+
 func Load(t testing.TB, exporter packagestest.Exporter, dir string) *Data {
 	t.Helper()
 
@@ -128,19 +137,29 @@ func Load(t testing.TB, exporter packagestest.Exporter, dir string) *Data {
 		t:         t,
 		dir:       dir,
 		fragments: map[string]string{},
+		golden:    map[string]*Golden{},
 	}
 
 	files := packagestest.MustCopyFileTree(dir)
 	overlays := map[string][]byte{}
 	for fragment, operation := range files {
-		if strings.Contains(fragment, goldenFile) {
+		if trimmed := strings.TrimSuffix(fragment, goldenFileSuffix); trimmed != fragment {
 			delete(files, fragment)
-		} else if trimmed := strings.TrimSuffix(fragment, inFile); trimmed != fragment {
+			goldFile := filepath.Join(dir, fragment)
+			archive, err := txtar.ParseFile(goldFile)
+			if err != nil {
+				t.Fatalf("could not read golden file %v: %v", fragment, err)
+			}
+			data.golden[trimmed] = &Golden{
+				Filename: goldFile,
+				Archive:  archive,
+			}
+		} else if trimmed := strings.TrimSuffix(fragment, inFileSuffix); trimmed != fragment {
 			delete(files, fragment)
 			files[trimmed] = operation
-		} else if index := strings.Index(fragment, overlayFile); index >= 0 {
+		} else if index := strings.Index(fragment, overlayFileSuffix); index >= 0 {
 			delete(files, fragment)
-			partial := fragment[:index] + fragment[index+len(overlayFile):]
+			partial := fragment[:index] + fragment[index+len(overlayFileSuffix):]
 			contents, err := ioutil.ReadFile(filepath.Join(dir, fragment))
 			if err != nil {
 				t.Fatal(err)
@@ -200,6 +219,12 @@ func Load(t testing.TB, exporter packagestest.Exporter, dir string) *Data {
 			symbols[i].Children = children
 		}
 	}
+	// run a second pass to collect names for some entries.
+	if err := data.Exported.Expect(map[string]interface{}{
+		"godef": data.collectDefinitionNames,
+	}); err != nil {
+		t.Fatal(err)
+	}
 	return data
 }
 
@@ -287,9 +312,23 @@ func Run(t *testing.T, tests Tests, data *Data) {
 		}
 		tests.Link(t, data.Links)
 	})
+
+	if *updateGolden {
+		for _, golden := range data.golden {
+			if !golden.Modified {
+				continue
+			}
+			sort.Slice(golden.Archive.Files, func(i, j int) bool {
+				return golden.Archive.Files[i].Name < golden.Archive.Files[j].Name
+			})
+			if err := ioutil.WriteFile(golden.Filename, txtar.Format(golden.Archive), 0666); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
 }
 
-func (data *Data) Golden(tag string, target string, update func(golden string) error) []byte {
+func (data *Data) Golden(tag string, target string, update func() ([]byte, error)) []byte {
 	data.t.Helper()
 	fragment, found := data.fragments[target]
 	if !found {
@@ -298,24 +337,44 @@ func (data *Data) Golden(tag string, target string, update func(golden string) e
 		}
 		fragment = target
 	}
-	dir, file := path.Split(fragment)
+	golden := data.golden[fragment]
-	prefix, suffix := file, ""
+	if golden == nil {
-	// we deliberately use the first . not the last
+		if !*updateGolden {
-	if dot := strings.IndexRune(file, '.'); dot >= 0 {
+			data.t.Fatalf("could not find golden file %v: %v", fragment, tag)
-		prefix = file[:dot]
+		}
-		suffix = file[dot:]
+		golden = &Golden{
+			Filename: filepath.Join(data.dir, fragment+goldenFileSuffix),
+			Archive:  &txtar.Archive{},
+			Modified: true,
+		}
+		data.golden[fragment] = golden
 	}
-	golden := path.Join(data.dir, dir, prefix) + "." + tag + goldenFile + suffix
+	var file *txtar.File
-	if *updateGolden {
+	for i := range golden.Archive.Files {
-		if err := update(golden); err != nil {
+		f := &golden.Archive.Files[i]
-			data.t.Fatalf("could not update golden file %v: %v", golden, err)
+		if f.Name == tag {
+			file = f
+			break
 		}
 	}
-	contents, err := ioutil.ReadFile(golden)
+	if *updateGolden {
-	if err != nil {
+		if file == nil {
-		data.t.Fatalf("could not read golden file %v: %v", golden, err)
+			golden.Archive.Files = append(golden.Archive.Files, txtar.File{
+				Name: tag,
+			})
+			file = &golden.Archive.Files[len(golden.Archive.Files)-1]
+		}
+		contents, err := update()
+		if err != nil {
+			data.t.Fatalf("could not update golden file %v: %v", fragment, err)
+		}
+		file.Data = append(contents, '\n') // add trailing \n for txtar
+		golden.Modified = true
 	}
-	return contents
+	if file == nil {
+		data.t.Fatalf("could not find golden contents %v: %v", fragment, tag)
+	}
+	return file.Data[:len(file.Data)-1] // drop the trailing \n
 }
 
 func (data *Data) collectDiagnostics(spn span.Span, msgSource, msg string) {
@@ -371,6 +430,12 @@ func (data *Data) collectTypeDefinitions(src, target span.Span) {
 	}
 }
 
+func (data *Data) collectDefinitionNames(src span.Span, name string) {
+	d := data.Definitions[src]
+	d.Name = name
+	data.Definitions[src] = d
+}
+
 func (data *Data) collectHighlights(name string, rng span.Span) {
 	data.Highlights[name] = append(data.Highlights[name], rng)
 }