From cd1be5dbecf14f4fbe2e03e5949eea7b1467ec86 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 11 May 2021 19:00:06 -0400 Subject: [PATCH] gopls/internal/regtest: add a failing regtest for vscode-go#1489 Unimported completion computes invalid text edits with windows line endings. To enable this test, add support for windows line endings in the regtest framework. Doing this required decoupling the txtar encoding from the sandbox, which was a good change anyway. For golang/vscode-go#1489 Change-Id: I6c1075fd38d24090271a7a7f33b11ddd8f9decf5 Reviewed-on: https://go-review.googlesource.com/c/tools/+/319089 Trust: Robert Findley Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- .../regtest/completion/completion_test.go | 38 +++++++++++++++ internal/lsp/cache/view_test.go | 2 +- internal/lsp/cache/workspace_test.go | 2 +- internal/lsp/fake/editor.go | 47 ++++++++++++++----- internal/lsp/fake/editor_test.go | 2 +- internal/lsp/fake/proxy.go | 3 +- internal/lsp/fake/sandbox.go | 14 +++--- internal/lsp/fake/workdir.go | 5 +- internal/lsp/fake/workdir_test.go | 2 +- internal/lsp/lsprpc/lsprpc_test.go | 2 +- internal/lsp/regtest/runner.go | 14 +++++- 11 files changed, 99 insertions(+), 32 deletions(-) diff --git a/gopls/internal/regtest/completion/completion_test.go b/gopls/internal/regtest/completion/completion_test.go index d57a4b237b1..afdd494a8b1 100644 --- a/gopls/internal/regtest/completion/completion_test.go +++ b/gopls/internal/regtest/completion/completion_test.go @@ -503,3 +503,41 @@ func doit() { } }) } + +func TestUnimportedCompletion_VSCodeIssue1489(t *testing.T) { + t.Skip("broken due to golang/vscode-go#1489") + testenv.NeedsGo1Point(t, 14) + + const src = ` +-- go.mod -- +module mod.com + +go 1.14 + +-- main.go -- +package main + +import "fmt" + +func main() { + fmt.Println("a") + math.Sqr +} +` + WithOptions( + WindowsLineEndings, + ProxyFiles(proxy), + ).Run(t, src, func(t *testing.T, env *Env) { + // Trigger unimported completions for the example.com/blah package. + env.OpenFile("main.go") + env.Await(env.DoneWithOpen()) + pos := env.RegexpSearch("main.go", "Sqr()") + completions := env.Completion("main.go", pos) + if len(completions.Items) == 0 { + t.Fatalf("no completion items") + } + env.AcceptCompletion("main.go", pos, completions.Items[0]) + env.Await(env.DoneWithChange()) + t.Log(env.Editor.BufferText("main.go")) + }) +} diff --git a/internal/lsp/cache/view_test.go b/internal/lsp/cache/view_test.go index cb57182353f..802215a5aac 100644 --- a/internal/lsp/cache/view_test.go +++ b/internal/lsp/cache/view_test.go @@ -66,7 +66,7 @@ module de -- f/g/go.mod -- module fg ` - dir, err := fake.Tempdir(workspace) + dir, err := fake.Tempdir(fake.UnpackTxt(workspace)) if err != nil { t.Fatal(err) } diff --git a/internal/lsp/cache/workspace_test.go b/internal/lsp/cache/workspace_test.go index fd9cb8d1d3c..85240611645 100644 --- a/internal/lsp/cache/workspace_test.go +++ b/internal/lsp/cache/workspace_test.go @@ -269,7 +269,7 @@ replace gopls.test => ../../gopls.test2`, false}, for _, test := range tests { t.Run(test.desc, func(t *testing.T) { ctx := context.Background() - dir, err := fake.Tempdir(test.initial) + dir, err := fake.Tempdir(fake.UnpackTxt(test.initial)) if err != nil { t.Fatal(err) } diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index de01f0b38a8..501d32c1806 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -55,14 +55,19 @@ type CallCounts struct { } type buffer struct { - version int - path string - lines []string - dirty bool + windowsLineEndings bool + version int + path string + lines []string + dirty bool } func (b buffer) text() string { - return strings.Join(b.lines, "\n") + eol := "\n" + if b.windowsLineEndings { + eol = "\r\n" + } + return strings.Join(b.lines, eol) } // EditorConfig configures the editor's LSP session. This is similar to @@ -106,6 +111,9 @@ type EditorConfig struct { // the PID. This can only be set by one test. SendPID bool + // Whether to edit files with windows line endings. + WindowsLineEndings bool + DirectoryFilters []string VerboseOutput bool @@ -338,11 +346,11 @@ func (e *Editor) onFileChanges(ctx context.Context, evts []FileEvent) { continue // A race with some other operation. } // No need to update if the buffer content hasn't changed. - if content == strings.Join(buf.lines, "\n") { + if content == buf.text() { continue } // During shutdown, this call will fail. Ignore the error. - _ = e.setBufferContentLocked(ctx, evt.Path, false, strings.Split(content, "\n"), nil) + _ = e.setBufferContentLocked(ctx, evt.Path, false, lines(content), nil) } } e.Server.DidChangeWatchedFiles(ctx, &protocol.DidChangeWatchedFilesParams{ @@ -383,10 +391,11 @@ func (e *Editor) CreateBuffer(ctx context.Context, path, content string) error { func (e *Editor) createBuffer(ctx context.Context, path string, dirty bool, content string) error { buf := buffer{ - version: 1, - path: path, - lines: strings.Split(content, "\n"), - dirty: dirty, + windowsLineEndings: e.Config.WindowsLineEndings, + version: 1, + path: path, + lines: lines(content), + dirty: dirty, } e.mu.Lock() defer e.mu.Unlock() @@ -406,6 +415,15 @@ func (e *Editor) createBuffer(ctx context.Context, path string, dirty bool, cont return nil } +// lines returns line-ending agnostic line representation of content. +func lines(content string) []string { + lines := strings.Split(content, "\n") + for i, l := range lines { + lines[i] = strings.TrimSuffix(l, "\r") + } + return lines +} + // CloseBuffer removes the current buffer (regardless of whether it is saved). func (e *Editor) CloseBuffer(ctx context.Context, path string) error { e.mu.Lock() @@ -528,6 +546,7 @@ var ( // regexpRange returns the start and end of the first occurrence of either re // or its singular subgroup. It returns ErrNoMatch if the regexp doesn't match. func regexpRange(content, re string) (Pos, Pos, error) { + content = normalizeEOL(content) var start, end int rec, err := regexp.Compile(re) if err != nil { @@ -558,6 +577,10 @@ func regexpRange(content, re string) (Pos, Pos, error) { return startPos, endPos, nil } +func normalizeEOL(content string) string { + return strings.Join(lines(content), "\n") +} + // 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) { @@ -615,7 +638,7 @@ func (e *Editor) EditBuffer(ctx context.Context, path string, edits []Edit) erro func (e *Editor) SetBufferContent(ctx context.Context, path, content string) error { e.mu.Lock() defer e.mu.Unlock() - lines := strings.Split(content, "\n") + lines := lines(content) return e.setBufferContentLocked(ctx, path, true, lines, nil) } diff --git a/internal/lsp/fake/editor_test.go b/internal/lsp/fake/editor_test.go index f1ce7537ae5..3ce5df6e08f 100644 --- a/internal/lsp/fake/editor_test.go +++ b/internal/lsp/fake/editor_test.go @@ -48,7 +48,7 @@ func main() { ` func TestClientEditing(t *testing.T) { - ws, err := NewSandbox(&SandboxConfig{Files: exampleProgram}) + ws, err := NewSandbox(&SandboxConfig{Files: UnpackTxt(exampleProgram)}) if err != nil { t.Fatal(err) } diff --git a/internal/lsp/fake/proxy.go b/internal/lsp/fake/proxy.go index dbba27d76f7..9e56efeb17f 100644 --- a/internal/lsp/fake/proxy.go +++ b/internal/lsp/fake/proxy.go @@ -12,8 +12,7 @@ import ( // WriteProxy creates a new proxy file tree using the txtar-encoded content, // and returns its URL. -func WriteProxy(tmpdir, txt string) (string, error) { - files := unpackTxt(txt) +func WriteProxy(tmpdir string, files map[string][]byte) (string, error) { type moduleVersion struct { modulePath, version string } diff --git a/internal/lsp/fake/sandbox.go b/internal/lsp/fake/sandbox.go index c14f9035ac3..34f1ba180c0 100644 --- a/internal/lsp/fake/sandbox.go +++ b/internal/lsp/fake/sandbox.go @@ -38,7 +38,7 @@ type SandboxConfig struct { // // For convenience, the special substring "$SANDBOX_WORKDIR" is replaced with // the sandbox's resolved working directory before writing files. - Files string + Files map[string][]byte // InGoPath specifies that the working directory should be within the // temporary GOPATH. InGoPath bool @@ -51,10 +51,9 @@ type SandboxConfig struct { // // This option is incompatible with InGoPath or Files. Workdir string - // ProxyFiles holds a txtar-encoded archive of files to populate a file-based // Go proxy. - ProxyFiles string + ProxyFiles map[string][]byte // GOPROXY is the explicit GOPROXY value that should be used for the sandbox. // // This option is incompatible with ProxyFiles. @@ -141,12 +140,11 @@ func NewSandbox(config *SandboxConfig) (_ *Sandbox, err error) { // Tempdir creates a new temp directory with the given txtar-encoded files. It // is the responsibility of the caller to call os.RemoveAll on the returned // file path when it is no longer needed. -func Tempdir(txt string) (string, error) { +func Tempdir(files map[string][]byte) (string, error) { dir, err := ioutil.TempDir("", "gopls-tempdir-") if err != nil { return "", err } - files := unpackTxt(txt) for name, data := range files { if err := WriteFileData(name, data, RelativeTo(dir)); err != nil { return "", errors.Errorf("writing to tempdir: %w", err) @@ -155,7 +153,7 @@ func Tempdir(txt string) (string, error) { return dir, nil } -func unpackTxt(txt string) map[string][]byte { +func UnpackTxt(txt string) map[string][]byte { dataMap := make(map[string][]byte) archive := txtar.Parse([]byte(txt)) for _, f := range archive.Files { @@ -165,13 +163,13 @@ func unpackTxt(txt string) map[string][]byte { } func validateConfig(config SandboxConfig) error { - if filepath.IsAbs(config.Workdir) && (config.Files != "" || config.InGoPath) { + if filepath.IsAbs(config.Workdir) && (config.Files != nil || config.InGoPath) { return errors.New("absolute Workdir cannot be set in conjunction with Files or InGoPath") } if config.Workdir != "" && config.InGoPath { return errors.New("Workdir cannot be set in conjunction with InGoPath") } - if config.GOPROXY != "" && config.ProxyFiles != "" { + if config.GOPROXY != "" && config.ProxyFiles != nil { return errors.New("GOPROXY cannot be set in conjunction with ProxyFiles") } return nil diff --git a/internal/lsp/fake/workdir.go b/internal/lsp/fake/workdir.go index 5103bdb4e19..aa9ef84df67 100644 --- a/internal/lsp/fake/workdir.go +++ b/internal/lsp/fake/workdir.go @@ -50,7 +50,7 @@ func (r RelativeTo) RelPath(fp string) string { } func writeTxtar(txt string, rel RelativeTo) error { - files := unpackTxt(txt) + files := UnpackTxt(txt) for name, data := range files { if err := WriteFileData(name, data, rel); err != nil { return errors.Errorf("writing to workdir: %w", err) @@ -96,8 +96,7 @@ func hashFile(data []byte) string { return fmt.Sprintf("%x", sha256.Sum256(data)) } -func (w *Workdir) writeInitialFiles(txt string) error { - files := unpackTxt(txt) +func (w *Workdir) writeInitialFiles(files map[string][]byte) error { w.files = map[string]string{} for name, data := range files { w.files[name] = hashFile(data) diff --git a/internal/lsp/fake/workdir_test.go b/internal/lsp/fake/workdir_test.go index f57ea37e13b..33fbb9fa1d5 100644 --- a/internal/lsp/fake/workdir_test.go +++ b/internal/lsp/fake/workdir_test.go @@ -30,7 +30,7 @@ func newWorkdir(t *testing.T) (*Workdir, <-chan []FileEvent, func()) { t.Fatal(err) } wd := NewWorkdir(tmpdir) - if err := wd.writeInitialFiles(data); err != nil { + if err := wd.writeInitialFiles(UnpackTxt(data)); err != nil { t.Fatal(err) } cleanup := func() { diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go index 5fa6b9715f5..ef2555d9f1c 100644 --- a/internal/lsp/lsprpc/lsprpc_test.go +++ b/internal/lsp/lsprpc/lsprpc_test.go @@ -197,7 +197,7 @@ func main() { }` func TestDebugInfoLifecycle(t *testing.T) { - sb, err := fake.NewSandbox(&fake.SandboxConfig{Files: exampleProgram}) + sb, err := fake.NewSandbox(&fake.SandboxConfig{Files: fake.UnpackTxt(exampleProgram)}) if err != nil { t.Fatal(err) } diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go index 87cfa6de63a..5eeacd8634e 100644 --- a/internal/lsp/regtest/runner.go +++ b/internal/lsp/regtest/runner.go @@ -109,7 +109,7 @@ func Timeout(d time.Duration) RunOption { // ProxyFiles configures a file proxy using the given txtar-encoded string. func ProxyFiles(txt string) RunOption { return optionSetter(func(opts *runConfig) { - opts.sandbox.ProxyFiles = txt + opts.sandbox.ProxyFiles = fake.UnpackTxt(txt) }) } @@ -177,6 +177,10 @@ func DebugAddress(addr string) RunOption { }) } +var WindowsLineEndings = optionSetter(func(opts *runConfig) { + opts.editor.WindowsLineEndings = true +}) + // SkipLogs skips the buffering of logs during test execution. It is intended // for long-running stress tests. func SkipLogs() RunOption { @@ -269,6 +273,12 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio if err := os.MkdirAll(rootDir, 0755); err != nil { t.Fatal(err) } + files := fake.UnpackTxt(files) + if config.editor.WindowsLineEndings { + for name, data := range files { + files[name] = bytes.ReplaceAll(data, []byte("\n"), []byte("\r\n")) + } + } config.sandbox.Files = files config.sandbox.RootDir = rootDir sandbox, err := fake.NewSandbox(&config.sandbox) @@ -385,7 +395,7 @@ func singletonServer(ctx context.Context, t *testing.T, optsHook func(*source.Op return lsprpc.NewStreamServer(cache.New(optsHook), false) } -func experimentalWorkspaceModule(_ context.Context, t *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { +func experimentalWorkspaceModule(_ context.Context, _ *testing.T, optsHook func(*source.Options)) jsonrpc2.StreamServer { options := func(o *source.Options) { optsHook(o) o.ExperimentalWorkspaceModule = true