internal/lsp/regtest: use a common directory for regtest sandboxes

For easier debugging (and less cruft if regtests are ctrl-c'ed), root
all regtest sandboxes in a common directory.

This also tries one last time to clean up the directory, and fails on an
error. This might be flaky on windows, but hasn't been so far...

Also give regtest sandboxes names derived from their test name.

Updates golang/go#39384
Updates golang/go#38490

Change-Id: Iae53c29e75f5eb2b8d938d205fbeb463ffc82eb2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/240059
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

internal/lsp/fake/editor_test.go

@@ -48,7 +48,7 @@ func main() {
 `
 
 func TestClientEditing(t *testing.T) {
-	ws, err := NewSandbox("TestClientEditing", exampleProgram, "", false, false)
+	ws, err := NewSandbox("", exampleProgram, "", false, false)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/lsp/fake/sandbox.go

@@ -20,7 +20,6 @@ import (
 // Sandbox holds a collection of temporary resources to use for working with Go
 // code in tests.
 type Sandbox struct {
-	name    string
 	gopath  string
 	basedir string
 	Proxy   *Proxy
@@ -36,21 +35,24 @@ type Sandbox struct {
 // working directory populated by the txtar-encoded content in srctxt, and a
 // file-based module proxy populated with the txtar-encoded content in
 // proxytxt.
-func NewSandbox(name, srctxt, proxytxt string, inGopath bool, withoutWorkspaceFolders bool) (_ *Sandbox, err error) {
-	sb := &Sandbox{
-		name: name,
-	}
+//
+// If rootDir is non-empty, it will be used as the root of temporary
+// directories created for the sandbox. Otherwise, a new temporary directory
+// will be used as root.
+func NewSandbox(rootDir, srctxt, proxytxt string, inGopath bool, withoutWorkspaceFolders bool) (_ *Sandbox, err error) {
+	sb := &Sandbox{}
 	defer func() {
 		// Clean up if we fail at any point in this constructor.
 		if err != nil {
 			sb.Close()
 		}
 	}()
-	basedir, err := ioutil.TempDir("", fmt.Sprintf("goplstest-sandbox-%s-", name))
+
+	baseDir, err := ioutil.TempDir(rootDir, "gopls-sandbox-")
 	if err != nil {
 		return nil, fmt.Errorf("creating temporary workdir: %v", err)
 	}
-	sb.basedir = basedir
+	sb.basedir = baseDir
 	proxydir := filepath.Join(sb.basedir, "proxy")
 	sb.gopath = filepath.Join(sb.basedir, "gopath")
 	// Set the working directory as $GOPATH/src if inGopath is true.

internal/lsp/lsprpc/lsprpc_test.go

@@ -196,7 +196,7 @@ func main() {
 }`
 
 func TestDebugInfoLifecycle(t *testing.T) {
-	sb, err := fake.NewSandbox("gopls-lsprpc-test", exampleProgram, "", false, false)
+	sb, err := fake.NewSandbox("", exampleProgram, "", false, false)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/lsp/regtest/reg_test.go

@@ -8,6 +8,7 @@ import (
 	"context"
 	"flag"
 	"fmt"
+	"io/ioutil"
 	"os"
 	"testing"
 	"time"
@@ -20,6 +21,7 @@ var (
 	runSubprocessTests       = flag.Bool("enable_gopls_subprocess_tests", false, "run regtests against a gopls subprocess")
 	goplsBinaryPath          = flag.String("gopls_test_binary", "", "path to the gopls binary for use as a remote, for use with the -enable_gopls_subprocess_tests flag")
 	regtestTimeout           = flag.Duration("regtest_timeout", 60*time.Second, "default timeout for each regtest")
+	skipCleanup              = flag.Bool("regtest_skip_cleanup", false, "whether to skip cleaning up temp directories")
 	printGoroutinesOnFailure = flag.Bool("regtest_print_goroutines", false, "whether to print goroutines info on failure")
 )
 
@@ -36,6 +38,7 @@ func TestMain(m *testing.M) {
 		DefaultModes:             NormalModes,
 		Timeout:                  *regtestTimeout,
 		PrintGoroutinesOnFailure: *printGoroutinesOnFailure,
+		SkipCleanup:              *skipCleanup,
 	}
 	if *runSubprocessTests {
 		goplsPath := *goplsBinaryPath
@@ -49,8 +52,16 @@ func TestMain(m *testing.M) {
 		runner.DefaultModes = NormalModes | SeparateProcess
 		runner.GoplsPath = goplsPath
 	}
+	dir, err := ioutil.TempDir("", "gopls-regtest-")
+	if err != nil {
+		panic(fmt.Errorf("creating regtest temp directory: %v", err))
+	}
+	runner.TempDir = dir
 
 	code := m.Run()
-	runner.Close()
+	if err := runner.Close(); err != nil {
+		fmt.Fprintf(os.Stderr, "closing test runner: %v\n", err)
+		os.Exit(1)
+	}
 	os.Exit(code)
 }

internal/lsp/regtest/runner.go

@@ -56,6 +56,8 @@ type Runner struct {
 	Timeout                  time.Duration
 	GoplsPath                string
 	PrintGoroutinesOnFailure bool
+	TempDir                  string
+	SkipCleanup              bool
 
 	mu        sync.Mutex
 	ts        *servertest.TCPServer
@@ -70,7 +72,6 @@ type runConfig struct {
 	modes                   Mode
 	proxyTxt                string
 	timeout                 time.Duration
-	skipCleanup             bool
 	gopath                  bool
 	withoutWorkspaceFolders bool
 }
@@ -139,14 +140,6 @@ func InGOPATH() RunOption {
 	})
 }
 
-// SkipCleanup is used only for debugging: is skips cleaning up the tests state
-// after completion.
-func SkipCleanup() RunOption {
-	return optionSetter(func(opts *runConfig) {
-		opts.skipCleanup = true
-	})
-}
-
 // Run executes the test function in the default configured gopls execution
 // modes. For each a test run, a new workspace is created containing the
 // un-txtared files specified by filedata.
@@ -173,12 +166,16 @@ func (r *Runner) Run(t *testing.T, filedata string, test func(t *testing.T, e *E
 			continue
 		}
 		t.Run(tc.name, func(t *testing.T) {
-			t.Helper()
 			ctx, cancel := context.WithTimeout(context.Background(), config.timeout)
 			defer cancel()
 			ctx = debug.WithInstance(ctx, "", "")
 
-			sandbox, err := fake.NewSandbox("regtest", filedata, config.proxyTxt, config.gopath, config.withoutWorkspaceFolders)
+			tempDir := filepath.Join(r.TempDir, filepath.FromSlash(t.Name()))
+			if err := os.MkdirAll(tempDir, 0755); err != nil {
+				t.Fatal(err)
+			}
+
+			sandbox, err := fake.NewSandbox(tempDir, filedata, config.proxyTxt, config.gopath, config.withoutWorkspaceFolders)
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -188,13 +185,7 @@ func (r *Runner) Run(t *testing.T, filedata string, test func(t *testing.T, e *E
 			// Windows. This may still be flaky however, and in the future we need a
 			// better solution to ensure that all Go processes started by gopls have
 			// exited before we clean up.
-			if config.skipCleanup {
-				defer func() {
-					t.Logf("Skipping workspace cleanup: running in %s", sandbox.Workdir.RootURI())
-				}()
-			} else {
-				r.AddCloser(sandbox)
-			}
+			r.AddCloser(sandbox)
 			ss := tc.getServer(ctx, t)
 			ls := &loggingFramer{}
 			framer := ls.framer(jsonrpc2.NewRawStream)
@@ -291,7 +282,7 @@ func (r *Runner) getRemoteSocket(t *testing.T) string {
 		t.Fatal("cannot run tests with a separate process unless a path to a gopls binary is configured")
 	}
 	var err error
-	r.socketDir, err = ioutil.TempDir("", "gopls-regtests")
+	r.socketDir, err = ioutil.TempDir(r.TempDir, "gopls-regtest-socket")
 	if err != nil {
 		t.Fatalf("creating tempdir: %v", err)
 	}
@@ -333,8 +324,13 @@ func (r *Runner) Close() error {
 			errmsgs = append(errmsgs, err.Error())
 		}
 	}
-	for _, closer := range r.closers {
-		if err := closer.Close(); err != nil {
+	if !r.SkipCleanup {
+		for _, closer := range r.closers {
+			if err := closer.Close(); err != nil {
+				errmsgs = append(errmsgs, err.Error())
+			}
+		}
+		if err := os.RemoveAll(r.TempDir); err != nil {
 			errmsgs = append(errmsgs, err.Error())
 		}
 	}