Skip to content

Commit 248c34b

Browse files
adonovangopherbot
adonovan
authored and
gopherbot
committed
internal/lsp: support regular expressions in Diagnostics tests
The diagnostics emitted by the Go toolchain vary from one release to the next, and the gopls tests run against multiple versions. Therefore our tests need a way to express multiple possibilities in their expectations. This change interprets the patterns in @diag test annotations as regular expressions, allowing some flexibility in recognizing both old and new messages. It's not a panacea but it is one step to reducing the considerable friction of making changes to the compiler or go/types in the main tree. Details: - Factor the three implementations of the Tests.Diagnostics abstract method so that they all use DiffDiagnostics to report differences, substantially rewriting one of them. - Move the "no_diagnostics" hack, of which there were three copies, not all consistent, into DiffDiagnostics. - Eliminate the named type for each tests.Data field; a type alias is all that's needed. - Add Diagnostics.String method. - Add various TODOs for further improvements. - Add various apparently missing Fatal statements within the tests. Change-Id: Id38ad72a851b551dd4eb1d8c021bcb8adbb2213f Reviewed-on: https://go-review.googlesource.com/c/tools/+/425956 Run-TryBot: Alan Donovan <adonovan@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Robert Griesemer <gri@google.com> Reviewed-by: Robert Findley <rfindley@google.com> Auto-Submit: Alan Donovan <adonovan@google.com>
1 parent 431f4ef commit 248c34b

File tree

8 files changed

+120
-109
lines changed

8 files changed

+120
-109
lines changed

‎internal/lsp/cmd/test/check.go

Lines changed: 40 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -5,59 +5,61 @@
55
package cmdtest
66

77
import (
8-
"fmt"
98
"io/ioutil"
109
"strings"
1110
"testing"
1211

12+
"golang.org/x/tools/internal/lsp/protocol"
1313
"golang.org/x/tools/internal/lsp/source"
14+
"golang.org/x/tools/internal/lsp/tests"
1415
"golang.org/x/tools/internal/span"
1516
)
1617

18+
// Diagnostics runs the gopls command on a single file, parses its
19+
// diagnostics, and compares against the expectations defined by
20+
// markers in the source file.
1721
func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnostic) {
18-
if len(want) == 1 && want[0].Message == "" {
19-
return
22+
out, _ := r.runGoplsCmd(t, "check", uri.Filename())
23+
24+
content, err := ioutil.ReadFile(uri.Filename())
25+
if err != nil {
26+
t.Fatal(err)
2027
}
21-
fname := uri.Filename()
22-
out, _ := r.runGoplsCmd(t, "check", fname)
23-
// parse got into a collection of reports
24-
got := map[string]struct{}{}
25-
for _, l := range strings.Split(out, "\n") {
26-
if len(l) == 0 {
27-
continue
28+
mapper := protocol.NewColumnMapper(uri, content)
29+
30+
// Parse command output into a set of diagnostics.
31+
var got []*source.Diagnostic
32+
for _, line := range strings.Split(out, "\n") {
33+
if line == "" {
34+
continue // skip blank
35+
}
36+
parts := strings.SplitN(line, ": ", 2) // "span: message"
37+
if len(parts) != 2 {
38+
t.Fatalf("output line not of form 'span: message': %q", line)
2839
}
29-
// parse and reprint to normalize the span
30-
bits := strings.SplitN(l, ": ", 2)
31-
if len(bits) == 2 {
32-
spn := span.Parse(strings.TrimSpace(bits[0]))
33-
spn = span.New(spn.URI(), spn.Start(), span.Point{})
34-
data, err := ioutil.ReadFile(fname)
35-
if err != nil {
36-
t.Fatal(err)
37-
}
38-
converter := span.NewTokenFile(fname, data)
39-
s, err := spn.WithPosition(converter)
40-
if err != nil {
41-
t.Fatal(err)
42-
}
43-
l = fmt.Sprintf("%s: %s", s, strings.TrimSpace(bits[1]))
40+
spn, message := span.Parse(parts[0]), parts[1]
41+
rng, err := mapper.Range(spn)
42+
if err != nil {
43+
t.Fatal(err)
4444
}
45-
got[r.NormalizePrefix(l)] = struct{}{}
45+
// Set only the fields needed by DiffDiagnostics.
46+
got = append(got, &source.Diagnostic{
47+
URI: uri,
48+
Range: rng,
49+
Message: message,
50+
})
4651
}
52+
53+
// Don't expect fields that we can't populate from the command output.
4754
for _, diag := range want {
48-
expect := fmt.Sprintf("%v:%v:%v: %v", uri.Filename(), diag.Range.Start.Line+1, diag.Range.Start.Character+1, diag.Message)
49-
if diag.Range.Start.Character == 0 {
50-
expect = fmt.Sprintf("%v:%v: %v", uri.Filename(), diag.Range.Start.Line+1, diag.Message)
51-
}
52-
expect = r.NormalizePrefix(expect)
53-
_, found := got[expect]
54-
if !found {
55-
t.Errorf("missing diagnostic %q, %v", expect, got)
56-
} else {
57-
delete(got, expect)
55+
if diag.Source == "no_diagnostics" {
56+
continue // see DiffDiagnostics
5857
}
58+
diag.Source = ""
59+
diag.Severity = 0
5960
}
60-
for extra := range got {
61-
t.Errorf("extra diagnostic %q", extra)
61+
62+
if diff := tests.DiffDiagnostics(uri, want, got); diff != "" {
63+
t.Error(diff)
6264
}
6365
}

‎internal/lsp/lsp_test.go

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -211,16 +211,7 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnost
211211
// Get the diagnostics for this view if we have not done it before.
212212
v := r.server.session.View(r.data.Config.Dir)
213213
r.collectDiagnostics(v)
214-
d := r.diagnostics[uri]
215-
got := make([]*source.Diagnostic, len(d))
216-
copy(got, d)
217-
// A special case to test that there are no diagnostics for a file.
218-
if len(want) == 1 && want[0].Source == "no_diagnostics" {
219-
if len(got) != 0 {
220-
t.Errorf("expected no diagnostics for %s, got %v", uri, got)
221-
}
222-
return
223-
}
214+
got := append([]*source.Diagnostic(nil), r.diagnostics[uri]...) // copy
224215
if diff := tests.DiffDiagnostics(uri, want, got); diff != "" {
225216
t.Error(diff)
226217
}

‎internal/lsp/source/source_test.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -156,13 +156,6 @@ func (r *runner) Diagnostics(t *testing.T, uri span.URI, want []*source.Diagnost
156156
if err != nil {
157157
t.Fatal(err)
158158
}
159-
// A special case to test that there are no diagnostics for a file.
160-
if len(want) == 1 && want[0].Source == "no_diagnostics" {
161-
if len(got) != 0 {
162-
t.Errorf("expected no diagnostics for %s, got %v", uri, got)
163-
}
164-
return
165-
}
166159
if diff := tests.DiffDiagnostics(fileID.URI, want, got); diff != "" {
167160
t.Error(diff)
168161
}

‎internal/lsp/source/view.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -685,6 +685,10 @@ type Diagnostic struct {
685685
Analyzer *Analyzer
686686
}
687687

688+
func (d *Diagnostic) String() string {
689+
return fmt.Sprintf("%v: %s", d.Range, d.Message)
690+
}
691+
688692
type DiagnosticSource string
689693

690694
const (

‎internal/lsp/testdata/bad/bad0.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
1+
//go:build go1.11
12
// +build go1.11
23

34
package bad
45

5-
import _ "golang.org/x/tools/internal/lsp/assign/internal/secret" //@diag("\"golang.org/x/tools/internal/lsp/assign/internal/secret\"", "compiler", "could not import golang.org/x/tools/internal/lsp/assign/internal/secret (invalid use of internal package golang.org/x/tools/internal/lsp/assign/internal/secret)", "error")
6+
import _ "golang.org/x/tools/internal/lsp/assign/internal/secret" //@diag("\"golang.org/x/tools/internal/lsp/assign/internal/secret\"", "compiler", "could not import golang.org/x/tools/internal/lsp/assign/internal/secret \\(invalid use of internal package golang.org/x/tools/internal/lsp/assign/internal/secret\\)", "error")
67

78
func stuff() { //@item(stuff, "stuff", "func()", "func")
89
x := "heeeeyyyy"
9-
random2(x) //@diag("x", "compiler", "cannot use x (variable of type string) as int value in argument to random2", "error")
10+
random2(x) //@diag("x", "compiler", "cannot use x \\(variable of type string\\) as int value in argument to random2", "error")
1011
random2(1) //@complete("dom", random, random2, random3)
1112
y := 3 //@diag("y", "compiler", "y declared but not used", "error")
1213
}

‎internal/lsp/testdata/good/good1.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ func random2(y int) int { //@item(good_random2, "random2", "func(y int) int", "f
1414
//@complete("", good_y_param, types_import, good_random, good_random2, good_stuff)
1515
var b types.Bob = &types.X{} //@prepare("ypes","types", "types")
1616
if _, ok := b.(*types.X); ok { //@complete("X", X_struct, Y_struct, Bob_interface, CoolAlias)
17+
_ = 0 // suppress "empty branch" diagnostic
1718
}
1819

1920
return y

‎internal/lsp/tests/tests.go

Lines changed: 51 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -54,39 +54,41 @@ func init() {
5454

5555
var UpdateGolden = flag.Bool("golden", false, "Update golden files")
5656

57-
type CallHierarchy map[span.Span]*CallHierarchyResult
58-
type CodeLens map[span.URI][]protocol.CodeLens
59-
type Diagnostics map[span.URI][]*source.Diagnostic
60-
type CompletionItems map[token.Pos]*completion.CompletionItem
61-
type Completions map[span.Span][]Completion
62-
type CompletionSnippets map[span.Span][]CompletionSnippet
63-
type UnimportedCompletions map[span.Span][]Completion
64-
type DeepCompletions map[span.Span][]Completion
65-
type FuzzyCompletions map[span.Span][]Completion
66-
type CaseSensitiveCompletions map[span.Span][]Completion
67-
type RankCompletions map[span.Span][]Completion
68-
type FoldingRanges []span.Span
69-
type Formats []span.Span
70-
type Imports []span.Span
71-
type SemanticTokens []span.Span
72-
type SuggestedFixes map[span.Span][]SuggestedFix
73-
type FunctionExtractions map[span.Span]span.Span
74-
type MethodExtractions map[span.Span]span.Span
75-
type Definitions map[span.Span]Definition
76-
type Implementations map[span.Span][]span.Span
77-
type Highlights map[span.Span][]span.Span
78-
type References map[span.Span][]span.Span
79-
type Renames map[span.Span]string
80-
type PrepareRenames map[span.Span]*source.PrepareItem
81-
type Symbols map[span.URI][]protocol.DocumentSymbol
82-
type SymbolsChildren map[string][]protocol.DocumentSymbol
83-
type SymbolInformation map[span.Span]protocol.SymbolInformation
84-
type InlayHints []span.Span
85-
type WorkspaceSymbols map[WorkspaceSymbolsTestType]map[span.URI][]string
86-
type Signatures map[span.Span]*protocol.SignatureHelp
87-
type Links map[span.URI][]Link
88-
type AddImport map[span.URI]string
89-
type Hovers map[span.Span]string
57+
// These type names apparently avoid the need to repeat the
58+
// type in the field name and the make() expression.
59+
type CallHierarchy = map[span.Span]*CallHierarchyResult
60+
type CodeLens = map[span.URI][]protocol.CodeLens
61+
type Diagnostics = map[span.URI][]*source.Diagnostic
62+
type CompletionItems = map[token.Pos]*completion.CompletionItem
63+
type Completions = map[span.Span][]Completion
64+
type CompletionSnippets = map[span.Span][]CompletionSnippet
65+
type UnimportedCompletions = map[span.Span][]Completion
66+
type DeepCompletions = map[span.Span][]Completion
67+
type FuzzyCompletions = map[span.Span][]Completion
68+
type CaseSensitiveCompletions = map[span.Span][]Completion
69+
type RankCompletions = map[span.Span][]Completion
70+
type FoldingRanges = []span.Span
71+
type Formats = []span.Span
72+
type Imports = []span.Span
73+
type SemanticTokens = []span.Span
74+
type SuggestedFixes = map[span.Span][]SuggestedFix
75+
type FunctionExtractions = map[span.Span]span.Span
76+
type MethodExtractions = map[span.Span]span.Span
77+
type Definitions = map[span.Span]Definition
78+
type Implementations = map[span.Span][]span.Span
79+
type Highlights = map[span.Span][]span.Span
80+
type References = map[span.Span][]span.Span
81+
type Renames = map[span.Span]string
82+
type PrepareRenames = map[span.Span]*source.PrepareItem
83+
type Symbols = map[span.URI][]protocol.DocumentSymbol
84+
type SymbolsChildren = map[string][]protocol.DocumentSymbol
85+
type SymbolInformation = map[span.Span]protocol.SymbolInformation
86+
type InlayHints = []span.Span
87+
type WorkspaceSymbols = map[WorkspaceSymbolsTestType]map[span.URI][]string
88+
type Signatures = map[span.Span]*protocol.SignatureHelp
89+
type Links = map[span.URI][]Link
90+
type AddImport = map[span.URI]string
91+
type Hovers = map[span.Span]string
9092

9193
type Data struct {
9294
Config packages.Config
@@ -137,6 +139,12 @@ type Data struct {
137139
mappers map[span.URI]*protocol.ColumnMapper
138140
}
139141

142+
// TODO(adonovan): there are multiple implementations of this (undocumented)
143+
// interface, each of which must implement similar semantics. For example:
144+
// - *runner in ../cmd/test/check.go
145+
// - *runner in ../source/source_test.go
146+
// - *runner in ../lsp_test.go
147+
// Can we avoid this duplication?
140148
type Tests interface {
141149
CallHierarchy(*testing.T, span.Span, *CallHierarchyResult)
142150
CodeLens(*testing.T, span.URI, []protocol.CodeLens)
@@ -1084,11 +1092,11 @@ func (data *Data) collectCodeLens(spn span.Span, title, cmd string) {
10841092
}
10851093
m, err := data.Mapper(spn.URI())
10861094
if err != nil {
1087-
return
1095+
data.t.Fatalf("Mapper: %v", err)
10881096
}
10891097
rng, err := m.Range(spn)
10901098
if err != nil {
1091-
return
1099+
data.t.Fatalf("Range: %v", err)
10921100
}
10931101
data.CodeLens[spn.URI()] = append(data.CodeLens[spn.URI()], protocol.CodeLens{
10941102
Range: rng,
@@ -1099,18 +1107,16 @@ func (data *Data) collectCodeLens(spn span.Span, title, cmd string) {
10991107
})
11001108
}
11011109

1102-
func (data *Data) collectDiagnostics(spn span.Span, msgSource, msg, msgSeverity string) {
1103-
if _, ok := data.Diagnostics[spn.URI()]; !ok {
1104-
data.Diagnostics[spn.URI()] = []*source.Diagnostic{}
1105-
}
1110+
func (data *Data) collectDiagnostics(spn span.Span, msgSource, msgPattern, msgSeverity string) {
11061111
m, err := data.Mapper(spn.URI())
11071112
if err != nil {
1108-
return
1113+
data.t.Fatalf("Mapper: %v", err)
11091114
}
11101115
rng, err := m.Range(spn)
11111116
if err != nil {
1112-
return
1117+
data.t.Fatalf("Range: %v", err)
11131118
}
1119+
11141120
severity := protocol.SeverityError
11151121
switch msgSeverity {
11161122
case "error":
@@ -1122,14 +1128,13 @@ func (data *Data) collectDiagnostics(spn span.Span, msgSource, msg, msgSeverity
11221128
case "information":
11231129
severity = protocol.SeverityInformation
11241130
}
1125-
// This is not the correct way to do this, but it seems excessive to do the full conversion here.
1126-
want := &source.Diagnostic{
1131+
1132+
data.Diagnostics[spn.URI()] = append(data.Diagnostics[spn.URI()], &source.Diagnostic{
11271133
Range: rng,
11281134
Severity: severity,
11291135
Source: source.DiagnosticSource(msgSource),
1130-
Message: msg,
1131-
}
1132-
data.Diagnostics[spn.URI()] = append(data.Diagnostics[spn.URI()], want)
1136+
Message: msgPattern,
1137+
})
11331138
}
11341139

11351140
func (data *Data) collectCompletions(typ CompletionTestType) func(span.Span, []token.Pos) {

‎internal/lsp/tests/util.go

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"fmt"
1111
"go/token"
1212
"path/filepath"
13+
"regexp"
1314
"sort"
1415
"strconv"
1516
"strings"
@@ -110,27 +111,40 @@ func summarizeSymbols(i int, want, got []protocol.DocumentSymbol, reason string,
110111
}
111112

112113
// DiffDiagnostics prints the diff between expected and actual diagnostics test
113-
// results.
114+
// results. If the sole expectation is "no_diagnostics", the check is suppressed.
115+
// The Message field of each want element must be a regular expression.
114116
func DiffDiagnostics(uri span.URI, want, got []*source.Diagnostic) string {
117+
// A special case to test that there are no diagnostics for a file.
118+
if len(want) == 1 && want[0].Source == "no_diagnostics" {
119+
if len(got) != 0 {
120+
return fmt.Sprintf("expected no diagnostics for %s, got %v", uri, got)
121+
}
122+
return ""
123+
}
124+
115125
source.SortDiagnostics(want)
116126
source.SortDiagnostics(got)
117127

118128
if len(got) != len(want) {
129+
// TODO(adonovan): print the actual difference, not the difference in length!
119130
return summarizeDiagnostics(-1, uri, want, got, "different lengths got %v want %v", len(got), len(want))
120131
}
121132
for i, w := range want {
122133
g := got[i]
123-
if w.Message != g.Message {
124-
return summarizeDiagnostics(i, uri, want, got, "incorrect Message got %v want %v", g.Message, w.Message)
134+
if match, err := regexp.MatchString(w.Message, g.Message); err != nil {
135+
return summarizeDiagnostics(i, uri, want, got, "invalid regular expression %q: %v", w.Message, err)
136+
137+
} else if !match {
138+
return summarizeDiagnostics(i, uri, want, got, "got Message %q, want match for pattern %q", g.Message, w.Message)
125139
}
126140
if w.Severity != g.Severity {
127-
return summarizeDiagnostics(i, uri, want, got, "incorrect Severity got %v want %v", g.Severity, w.Severity)
141+
return summarizeDiagnostics(i, uri, want, got, "got Severity %v, want %v", g.Severity, w.Severity)
128142
}
129143
if w.Source != g.Source {
130-
return summarizeDiagnostics(i, uri, want, got, "incorrect Source got %v want %v", g.Source, w.Source)
144+
return summarizeDiagnostics(i, uri, want, got, "got Source %v, want %v", g.Source, w.Source)
131145
}
132146
if !rangeOverlaps(g.Range, w.Range) {
133-
return summarizeDiagnostics(i, uri, want, got, "range %v does not overlap %v", g.Range, w.Range)
147+
return summarizeDiagnostics(i, uri, want, got, "got Range %v, want overlap with %v", g.Range, w.Range)
134148
}
135149
}
136150
return ""

0 commit comments

Comments
 (0)