From 75ebdcb73b49a8a275acf576f7ee61eee365c85b Mon Sep 17 00:00:00 2001 From: Danish Dua Date: Wed, 16 Sep 2020 12:08:38 -0400 Subject: [PATCH] gopls/internal/regtest: add expected ranges in completion tests This change adds tests for package completion in multiline comments and package completion on a newline. It also adds expected ranges in completion tests since simply testing for returned item labels won't catch regression. For more info, see the discussion at https://go-review.googlesource.com/c/tools/+/253177. Change-Id: Ia42e2645e26647617b32f676952767d2da90e427 Reviewed-on: https://go-review.googlesource.com/c/tools/+/255278 Run-TryBot: Danish Dua gopls-CI: kokoro TryBot-Result: Go Bot Trust: Danish Dua Reviewed-by: Heschi Kreinick --- gopls/internal/regtest/completion_test.go | 116 ++++++++++++++-------- gopls/internal/regtest/wrappers.go | 15 +++ internal/lsp/fake/editor.go | 22 ++-- internal/lsp/fake/workdir.go | 8 ++ 4 files changed, 114 insertions(+), 47 deletions(-) diff --git a/gopls/internal/regtest/completion_test.go b/gopls/internal/regtest/completion_test.go index b21ff44be2..7cc0b6ab9f 100644 --- a/gopls/internal/regtest/completion_test.go +++ b/gopls/internal/regtest/completion_test.go @@ -31,6 +31,10 @@ fun apple() int { -- fruits/testfile.go -- // this is a comment +/* + this is a multiline comment +*/ + import "fmt" func test() {} @@ -44,57 +48,78 @@ pac var ( testfile4 = "" testfile5 = "/*a comment*/ " + testfile6 = "/*a comment*/\n" ) for _, tc := range []struct { - name string - filename string - content *string - line, col int - want []string + name string + filename string + content *string + triggerRegexp string + want []string + editRegexp string }{ { - name: "package completion at valid position", - filename: "fruits/testfile.go", - line: 1, col: 0, - want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + name: "package completion at valid position", + filename: "fruits/testfile.go", + triggerRegexp: "\n()", + want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + editRegexp: "\n()", }, { - name: "package completion in a comment", - filename: "fruits/testfile.go", - line: 0, col: 5, - want: nil, + name: "package completion in a comment", + filename: "fruits/testfile.go", + triggerRegexp: "th(i)s", + want: nil, }, { - name: "package completion at invalid position", - filename: "fruits/testfile.go", - line: 4, col: 0, - want: nil, + name: "package completion in a multiline comment", + filename: "fruits/testfile.go", + triggerRegexp: `\/\*\n()`, + want: nil, }, { - name: "package completion after keyword 'package'", - filename: "fruits/testfile2.go", - line: 0, col: 7, - want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + name: "package completion at invalid position", + filename: "fruits/testfile.go", + triggerRegexp: "import \"fmt\"\n()", + want: nil, }, { - name: "package completion with 'pac' prefix", - filename: "fruits/testfile3.go", - line: 0, col: 3, - want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + name: "package completion after keyword 'package'", + filename: "fruits/testfile2.go", + triggerRegexp: "package()", + want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + editRegexp: "package\n", }, { - name: "package completion at end of file", - filename: "fruits/testfile4.go", - line: 0, col: 0, - content: &testfile4, - want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + name: "package completion with 'pac' prefix", + filename: "fruits/testfile3.go", + triggerRegexp: "pac()", + want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + editRegexp: "pac", }, { - name: "package completion without terminal newline", - filename: "fruits/testfile5.go", - line: 0, col: 14, - content: &testfile5, - want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + name: "package completion for empty file", + filename: "fruits/testfile4.go", + triggerRegexp: "^$", + content: &testfile4, + want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + editRegexp: "^$", + }, + { + name: "package completion without terminal newline", + filename: "fruits/testfile5.go", + triggerRegexp: `\*\/ ()`, + content: &testfile5, + want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + editRegexp: `\*\/ ()`, + }, + { + name: "package completion on terminal newline", + filename: "fruits/testfile6.go", + triggerRegexp: `\*\/\n()`, + content: &testfile6, + want: []string{"package apple", "package apple_test", "package fruits", "package fruits_test", "package main"}, + editRegexp: `\*\/\n()`, }, } { t.Run(tc.name, func(t *testing.T) { @@ -106,10 +131,7 @@ pac ) } env.OpenFile(tc.filename) - completions, err := env.Editor.Completion(env.Ctx, tc.filename, fake.Pos{ - Line: tc.line, - Column: tc.col, - }) + completions, err := env.Editor.Completion(env.Ctx, tc.filename, env.RegexpSearch(tc.filename, tc.triggerRegexp)) if err != nil { t.Fatal(err) } @@ -124,6 +146,22 @@ pac t.Fatalf("unexpected text edit range end line number: got %d, want less than %d", end, lineCount) } } + + if tc.want != nil { + start, end := env.RegexpRange(tc.filename, tc.editRegexp) + expectedRng := protocol.Range{ + Start: fake.Pos.ToProtocolPosition(start), + End: fake.Pos.ToProtocolPosition(end), + } + for _, item := range completions.Items { + gotRng := item.TextEdit.Range + if expectedRng != gotRng { + t.Errorf("unexpected completion range for completion item %s: got %v, want %v", + item.Label, gotRng, expectedRng) + } + } + } + diff := compareCompletionResults(tc.want, completions.Items) if diff != "" { t.Error(diff) diff --git a/gopls/internal/regtest/wrappers.go b/gopls/internal/regtest/wrappers.go index 7ac01a886a..d00712a55c 100644 --- a/gopls/internal/regtest/wrappers.go +++ b/gopls/internal/regtest/wrappers.go @@ -98,6 +98,21 @@ func (e *Env) EditBuffer(name string, edits ...fake.Edit) { } } +// RegexpRange returns the range of the first match for re in the buffer +// specified by name, calling t.Fatal on any error. It first searches for the +// position in open buffers, then in workspace files. +func (e *Env) RegexpRange(name, re string) (fake.Pos, fake.Pos) { + e.T.Helper() + start, end, err := e.Editor.RegexpRange(name, re) + if err == fake.ErrUnknownBuffer { + start, end, err = e.Sandbox.Workdir.RegexpRange(name, re) + } + if err != nil { + e.T.Fatalf("RegexpRange: %v, %v", name, err) + } + return start, end +} + // RegexpSearch returns the starting position of the first match for re in the // buffer specified by name, calling t.Fatal on any error. It first searches // for the position in open buffers, then in workspace files. diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index 892774c7e2..f9a5743778 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -426,7 +426,7 @@ func contentPosition(content string, offset int) (Pos, error) { return Pos{}, errors.Errorf("scanning content: %w", err) } // Scan() will drop the last line if it is empty. Correct for this. - if strings.HasSuffix(content, "\n") && offset == start { + if (strings.HasSuffix(content, "\n") || content == "") && offset == start { return Pos{Line: line, Column: 0}, nil } return Pos{}, fmt.Errorf("position %d out of bounds in %q (line = %d, start = %d)", offset, content, line, start) @@ -471,6 +471,18 @@ func regexpRange(content, re string) (Pos, Pos, error) { return startPos, endPos, nil } +// RegexpRange returns the first range in the buffer bufName matching re. See +// RegexpSearch for more information on matching. +func (e *Editor) RegexpRange(bufName, re string) (Pos, Pos, error) { + e.mu.Lock() + defer e.mu.Unlock() + buf, ok := e.buffers[bufName] + if !ok { + return Pos{}, Pos{}, ErrUnknownBuffer + } + return regexpRange(buf.text(), re) +} + // RegexpSearch returns the position of the first match for re in the buffer // bufName. For convenience, RegexpSearch supports the following two modes: // 1. If re has no subgroups, return the position of the match for re itself. @@ -478,13 +490,7 @@ func regexpRange(content, re string) (Pos, Pos, error) { // It returns an error re is invalid, has more than one subgroup, or doesn't // match the buffer. func (e *Editor) RegexpSearch(bufName, re string) (Pos, error) { - e.mu.Lock() - defer e.mu.Unlock() - buf, ok := e.buffers[bufName] - if !ok { - return Pos{}, ErrUnknownBuffer - } - start, _, err := regexpRange(buf.text(), re) + start, _, err := e.RegexpRange(bufName, re) return start, err } diff --git a/internal/lsp/fake/workdir.go b/internal/lsp/fake/workdir.go index fdac27f953..cb24fde231 100644 --- a/internal/lsp/fake/workdir.go +++ b/internal/lsp/fake/workdir.go @@ -115,6 +115,14 @@ func (w *Workdir) ReadFile(path string) (string, error) { return string(b), nil } +func (w *Workdir) RegexpRange(path, re string) (Pos, Pos, error) { + content, err := w.ReadFile(path) + if err != nil { + return Pos{}, Pos{}, err + } + return regexpRange(content, re) +} + // RegexpSearch searches the file corresponding to path for the first position // matching re. func (w *Workdir) RegexpSearch(path string, re string) (Pos, error) {