From 53e0aa826876e42e86d5a53d2678ddb95fd4b996 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 24 Sep 2020 12:56:09 -0400 Subject: [PATCH] gopls/internal/regtest: add an option to nest the workdir Add a regtest run option NestWorkdir, which causes the working directory to be nested in the editor workspace, and use it in some modfile tests to exercise the new workspace logic. For now we only run the nested tests while using experimental workspace modules. In a later CL, the 'legacy' mode will be updated to find a solitary nested module, at which point we should be able to run nested in all modes. Fixes golang/go#42111 Change-Id: I0bd3b31969684bc1ba1935633cbb9a3f26de1587 Reviewed-on: https://go-review.googlesource.com/c/tools/+/257138 Run-TryBot: Robert Findley Reviewed-by: Rebecca Stambler Trust: Robert Findley --- gopls/internal/regtest/diagnostics_test.go | 2 +- gopls/internal/regtest/modfile_test.go | 39 ++++++---- gopls/internal/regtest/runner.go | 45 +++++++---- internal/lsp/fake/editor.go | 6 +- internal/lsp/fake/sandbox.go | 87 ++++++++++++++-------- 5 files changed, 114 insertions(+), 65 deletions(-) diff --git a/gopls/internal/regtest/diagnostics_test.go b/gopls/internal/regtest/diagnostics_test.go index d3685ddac1..e8190cf7dc 100644 --- a/gopls/internal/regtest/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics_test.go @@ -1246,7 +1246,7 @@ func main() { env.DiagnosticAtRegexp("main.go", "x"), ) }) - withOptions(WithRootPath("a"), WithLimitWorkspaceScope()).run(t, mod, func(t *testing.T, env *Env) { + withOptions(WithRootPath("a"), LimitWorkspaceScope()).run(t, mod, func(t *testing.T, env *Env) { env.OpenFile("a/main.go") env.Await( NoDiagnostics("main.go"), diff --git a/gopls/internal/regtest/modfile_test.go b/gopls/internal/regtest/modfile_test.go index 2f897a2fa9..22d780a36a 100644 --- a/gopls/internal/regtest/modfile_test.go +++ b/gopls/internal/regtest/modfile_test.go @@ -33,6 +33,15 @@ package hello const Name = "Hello" ` +func runModfileTest(t *testing.T, files, proxy string, f TestFunc) { + t.Run("normal", func(t *testing.T) { + withOptions(WithProxyFiles(proxy)).run(t, files, f) + }) + t.Run("nested", func(t *testing.T) { + withOptions(WithProxyFiles(proxy), NestWorkdir(), WithModes(Experimental)).run(t, files, f) + }) +} + func TestModFileModification(t *testing.T) { testenv.NeedsGo1Point(t, 14) @@ -50,7 +59,7 @@ func main() { } ` t.Run("basic", func(t *testing.T) { - withOptions(WithProxyFiles(proxy)).run(t, untidyModule, func(t *testing.T, env *Env) { + runModfileTest(t, untidyModule, proxy, func(t *testing.T, env *Env) { // Open the file and make sure that the initial workspace load does not // modify the go.mod file. goModContent := env.ReadWorkspaceFile("go.mod") @@ -77,7 +86,7 @@ func main() { t.Run("delete main.go", func(t *testing.T) { t.Skip("This test will be flaky until golang/go#40269 is resolved.") - withOptions(WithProxyFiles(proxy)).run(t, untidyModule, func(t *testing.T, env *Env) { + runModfileTest(t, untidyModule, proxy, func(t *testing.T, env *Env) { goModContent := env.ReadWorkspaceFile("go.mod") mainContent := env.ReadWorkspaceFile("main.go") env.OpenFile("main.go") @@ -126,7 +135,7 @@ go 1.12 require random.org v1.2.3 ` - withOptions(WithProxyFiles(proxy)).run(t, mod, func(t *testing.T, env *Env) { + runModfileTest(t, mod, proxy, func(t *testing.T, env *Env) { env.OpenFile("main.go") var d protocol.PublishDiagnosticsParams env.Await( @@ -172,7 +181,7 @@ go 1.12 require example.com v1.2.3 ` - runner.Run(t, mod, func(t *testing.T, env *Env) { + runModfileTest(t, mod, proxy, func(t *testing.T, env *Env) { env.OpenFile("go.mod") var d protocol.PublishDiagnosticsParams env.Await( @@ -185,7 +194,7 @@ require example.com v1.2.3 if got := env.Editor.BufferText("go.mod"); got != want { t.Fatalf("unexpected go.mod content:\n%s", tests.Diff(want, got)) } - }, WithProxyFiles(proxy)) + }) } func TestUnusedDiag(t *testing.T) { @@ -212,7 +221,7 @@ func main() {} go 1.14 ` - withOptions(WithProxyFiles(proxy)).run(t, files, func(t *testing.T, env *Env) { + runModfileTest(t, files, proxy, func(t *testing.T, env *Env) { env.OpenFile("go.mod") var d protocol.PublishDiagnosticsParams env.Await( @@ -266,7 +275,7 @@ import ( func _() { caire.RemoveTempImage() }` - runner.Run(t, repro, func(t *testing.T, env *Env) { + runModfileTest(t, repro, proxy, func(t *testing.T, env *Env) { env.OpenFile("main.go") var d protocol.PublishDiagnosticsParams env.Await( @@ -288,7 +297,7 @@ require ( if got := env.ReadWorkspaceFile("go.mod"); got != want { t.Fatalf("TestNewDepWithUnusedDep failed:\n%s", tests.Diff(want, got)) } - }, WithProxyFiles(proxy)) + }) } // TODO: For this test to be effective, the sandbox's file watcher must respect @@ -310,13 +319,13 @@ package main func main() { fmt.Println(blah.Name) ` - runner.Run(t, mod, func(t *testing.T, env *Env) { + runModfileTest(t, mod, proxy, func(t *testing.T, env *Env) { env.Await(env.DiagnosticAtRegexp("go.mod", "require")) env.RunGoCommand("mod", "tidy") env.Await( EmptyDiagnostics("go.mod"), ) - }, WithProxyFiles(proxy)) + }) } // Tests golang/go#39784: a missing indirect dependency, necessary @@ -409,7 +418,7 @@ func main() { // Start from a bad state/bad IWL, and confirm that we recover. t.Run("bad", func(t *testing.T) { - runner.Run(t, unknown, func(t *testing.T, env *Env) { + runModfileTest(t, unknown, proxy, func(t *testing.T, env *Env) { env.OpenFile("go.mod") env.Await( env.DiagnosticAtRegexp("go.mod", "example.com v1.2.2"), @@ -419,7 +428,7 @@ func main() { env.Await( env.DiagnosticAtRegexp("main.go", "x = "), ) - }, WithProxyFiles(proxy)) + }) }) const known = ` @@ -441,7 +450,7 @@ func main() { // Start from a good state, transform to a bad state, and confirm that we // still recover. t.Run("good", func(t *testing.T) { - runner.Run(t, known, func(t *testing.T, env *Env) { + runModfileTest(t, known, proxy, func(t *testing.T, env *Env) { env.OpenFile("go.mod") env.Await( env.DiagnosticAtRegexp("main.go", "x = "), @@ -456,7 +465,7 @@ func main() { env.Await( env.DiagnosticAtRegexp("main.go", "x = "), ) - }, WithProxyFiles(proxy)) + }) }) } @@ -501,7 +510,7 @@ func main() { println(blah.Name) } ` - withOptions(WithProxyFiles(badProxy)).run(t, module, func(t *testing.T, env *Env) { + runModfileTest(t, module, badProxy, func(t *testing.T, env *Env) { env.OpenFile("go.mod") env.Await( env.DiagnosticAtRegexp("go.mod", "require example.com v1.2.3"), diff --git a/gopls/internal/regtest/runner.go b/gopls/internal/regtest/runner.go index 8c5efe0f17..b9d646e689 100644 --- a/gopls/internal/regtest/runner.go +++ b/gopls/internal/regtest/runner.go @@ -39,7 +39,6 @@ const ( // and communicates over pipes to mimic the gopls sidecar execution mode, // which communicates over stdin/stderr. Singleton Mode = 1 << iota - // Forwarded forwards connections to a shared in-process gopls instance. Forwarded // SeparateProcess forwards connection to a shared separate gopls process. @@ -77,13 +76,14 @@ type Runner struct { } type runConfig struct { - editor fake.EditorConfig - sandbox fake.SandboxConfig - modes Mode - timeout time.Duration - debugAddr string - skipLogs bool - skipHooks bool + editor fake.EditorConfig + sandbox fake.SandboxConfig + modes Mode + timeout time.Duration + debugAddr string + skipLogs bool + skipHooks bool + nestWorkdir bool } func (r *Runner) defaultConfig() *runConfig { @@ -153,7 +153,7 @@ func WithoutWorkspaceFolders() RunOption { // tests need to check other cases. func WithRootPath(path string) RunOption { return optionSetter(func(opts *runConfig) { - opts.editor.EditorRootPath = path + opts.editor.WorkspaceRoot = path }) } @@ -209,13 +209,21 @@ func WithGOPROXY(goproxy string) RunOption { }) } -// WithLimitWorkspaceScope sets the LimitWorkspaceScope configuration. -func WithLimitWorkspaceScope() RunOption { +// LimitWorkspaceScope sets the LimitWorkspaceScope configuration. +func LimitWorkspaceScope() RunOption { return optionSetter(func(opts *runConfig) { opts.editor.LimitWorkspaceScope = true }) } +// NestWorkdir inserts the sandbox working directory in a subdirectory of the +// editor workspace. +func NestWorkdir() RunOption { + return optionSetter(func(opts *runConfig) { + opts.nestWorkdir = true + }) +} + type TestFunc func(t *testing.T, env *Env) // Run executes the test function in the default configured gopls execution @@ -262,16 +270,25 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio di.MonitorMemory(ctx) } - tempDir := filepath.Join(r.TempDir, filepath.FromSlash(t.Name())) - if err := os.MkdirAll(tempDir, 0755); err != nil { + rootDir := filepath.Join(r.TempDir, filepath.FromSlash(t.Name())) + if err := os.MkdirAll(rootDir, 0755); err != nil { t.Fatal(err) } + if config.nestWorkdir { + config.sandbox.Workdir = "work/nested" + } config.sandbox.Files = files - config.sandbox.RootDir = tempDir + config.sandbox.RootDir = rootDir sandbox, err := fake.NewSandbox(&config.sandbox) if err != nil { t.Fatal(err) } + workdir := sandbox.Workdir.RootURI().SpanURI().Filename() + if config.nestWorkdir { + // Now that we know the actual workdir, set our workspace to be the + // parent directory. + config.editor.WorkspaceRoot = filepath.Clean(filepath.Join(workdir, "..")) + } // Deferring the closure of ws until the end of the entire test suite // has, in testing, given the LSP server time to properly shutdown and // release any file locks held in workspace, which is a problem on diff --git a/internal/lsp/fake/editor.go b/internal/lsp/fake/editor.go index 3cc1c061c8..fed01eca95 100644 --- a/internal/lsp/fake/editor.go +++ b/internal/lsp/fake/editor.go @@ -81,9 +81,9 @@ type EditorConfig struct { // workspace folders nor a root URI. WithoutWorkspaceFolders bool - // EditorRootPath specifies the root path of the workspace folder used when + // WorkspaceRoot specifies the root path of the workspace folder used when // initializing gopls in the sandbox. If empty, the Workdir is used. - EditorRootPath string + WorkspaceRoot string // EnableStaticcheck enables staticcheck analyzers. EnableStaticcheck bool @@ -121,7 +121,7 @@ func (e *Editor) Connect(ctx context.Context, conn jsonrpc2.Conn, hooks ClientHo protocol.Handlers( protocol.ClientHandler(e.client, jsonrpc2.MethodNotFound))) - if err := e.initialize(ctx, e.Config.WithoutWorkspaceFolders, e.Config.EditorRootPath); err != nil { + if err := e.initialize(ctx, e.Config.WithoutWorkspaceFolders, e.Config.WorkspaceRoot); err != nil { return nil, err } e.sandbox.Workdir.AddWatcher(e.onFileChanges) diff --git a/internal/lsp/fake/sandbox.go b/internal/lsp/fake/sandbox.go index f240f9b54a..3b92dc0c98 100644 --- a/internal/lsp/fake/sandbox.go +++ b/internal/lsp/fake/sandbox.go @@ -22,7 +22,7 @@ import ( // code in tests. type Sandbox struct { gopath string - basedir string + rootdir string goproxy string Workdir *Workdir } @@ -42,9 +42,12 @@ type SandboxConfig struct { // InGoPath specifies that the working directory should be within the // temporary GOPATH. InGoPath bool - // Workdir configures the working directory of the Sandbox, for running in a - // pre-existing directory. If unset, a new working directory will be created - // under RootDir. + // Workdir configures the working directory of the Sandbox. It behaves as + // follows: + // - if set to an absolute path, use that path as the working directory. + // - if set to a relative path, create and use that path relative to the + // sandbox. + // - if unset, default to a the 'work' subdirectory of the sandbox. // // This option is incompatible with InGoPath or Files. Workdir string @@ -70,13 +73,8 @@ func NewSandbox(config *SandboxConfig) (_ *Sandbox, err error) { if config == nil { config = new(SandboxConfig) } - - if config.Workdir != "" && (config.Files != "" || config.InGoPath) { - return nil, fmt.Errorf("invalid SandboxConfig: Workdir cannot be used in conjunction with Files or InGoPath. Got %+v", config) - } - - if config.GOPROXY != "" && config.ProxyFiles != "" { - return nil, fmt.Errorf("invalid SandboxConfig: GOPROXY cannot be set in conjunction with ProxyFiles. Got %+v", config) + if err := validateConfig(*config); err != nil { + return nil, fmt.Errorf("invalid SandboxConfig: %v", err) } sb := &Sandbox{} @@ -87,19 +85,22 @@ func NewSandbox(config *SandboxConfig) (_ *Sandbox, err error) { } }() - baseDir, err := ioutil.TempDir(config.RootDir, "gopls-sandbox-") - if err != nil { - return nil, fmt.Errorf("creating temporary workdir: %v", err) + rootDir := config.RootDir + if rootDir == "" { + rootDir, err = ioutil.TempDir(config.RootDir, "gopls-sandbox-") + if err != nil { + return nil, fmt.Errorf("creating temporary workdir: %v", err) + } } - sb.basedir = baseDir - sb.gopath = filepath.Join(sb.basedir, "gopath") + sb.rootdir = rootDir + sb.gopath = filepath.Join(sb.rootdir, "gopath") if err := os.Mkdir(sb.gopath, 0755); err != nil { return nil, err } if config.GOPROXY != "" { sb.goproxy = config.GOPROXY } else { - proxydir := filepath.Join(sb.basedir, "proxy") + proxydir := filepath.Join(sb.rootdir, "proxy") if err := os.Mkdir(proxydir, 0755); err != nil { return nil, err } @@ -108,27 +109,32 @@ func NewSandbox(config *SandboxConfig) (_ *Sandbox, err error) { return nil, err } } - if config.Workdir != "" { + // Short-circuit writing the workdir if we're given an absolute path, since + // this is used for running in an existing directory. + // TODO(findleyr): refactor this to be less of a workaround. + if filepath.IsAbs(config.Workdir) { sb.Workdir = NewWorkdir(config.Workdir) - } else { - workdir := config.Workdir - // If we don't have a pre-existing work dir, we want to create either - // $GOPATH/src or . + return sb, nil + } + var workdir string + if config.Workdir == "" { if config.InGoPath { // Set the working directory as $GOPATH/src. workdir = filepath.Join(sb.gopath, "src") } else if workdir == "" { - workdir = filepath.Join(sb.basedir, "work") - } - if err := os.Mkdir(workdir, 0755); err != nil { - return nil, err - } - sb.Workdir = NewWorkdir(workdir) - if err := sb.Workdir.writeInitialFiles(config.Files); err != nil { - return nil, err + workdir = filepath.Join(sb.rootdir, "work") } + } else { + // relative path + workdir = filepath.Join(sb.rootdir, config.Workdir) + } + if err := os.MkdirAll(workdir, 0755); err != nil { + return nil, err + } + sb.Workdir = NewWorkdir(workdir) + if err := sb.Workdir.writeInitialFiles(config.Files); err != nil { + return nil, err } - return sb, nil } @@ -155,6 +161,19 @@ func unpackTxt(txt string) map[string][]byte { return dataMap } +func validateConfig(config SandboxConfig) error { + if filepath.IsAbs(config.Workdir) && (config.Files != "" || 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 != "" { + return errors.New("GOPROXY cannot be set in conjunction with ProxyFiles") + } + return nil +} + // splitModuleVersionPath extracts module information from files stored in the // directory structure modulePath@version/suffix. // For example: @@ -174,6 +193,10 @@ func splitModuleVersionPath(path string) (modulePath, version, suffix string) { return path, "", "" } +func (sb *Sandbox) RootDir() string { + return sb.rootdir +} + // GOPATH returns the value of the Sandbox GOPATH. func (sb *Sandbox) GOPATH() string { return sb.gopath @@ -236,7 +259,7 @@ func (sb *Sandbox) Close() error { if sb.gopath != "" { goCleanErr = sb.RunGoCommand(context.Background(), "", "clean", []string{"-modcache"}) } - err := os.RemoveAll(sb.basedir) + err := os.RemoveAll(sb.rootdir) if err != nil || goCleanErr != nil { return fmt.Errorf("error(s) cleaning sandbox: cleaning modcache: %v; removing files: %v", goCleanErr, err) }