From e404ca24fbdeb3f08cd930b3f371344b1b3f464b Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Thu, 25 Jun 2020 16:04:28 -0400 Subject: [PATCH] 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 TryBot-Result: Gobot Gobot Reviewed-by: Rebecca Stambler --- internal/lsp/fake/editor_test.go | 2 +- internal/lsp/fake/sandbox.go | 16 +++++++------ internal/lsp/lsprpc/lsprpc_test.go | 2 +- internal/lsp/regtest/reg_test.go | 13 +++++++++- internal/lsp/regtest/runner.go | 38 +++++++++++++----------------- 5 files changed, 40 insertions(+), 31 deletions(-) diff --git a/internal/lsp/fake/editor_test.go b/internal/lsp/fake/editor_test.go index bb5e19ef3a..df362228e4 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("TestClientEditing", exampleProgram, "", false, false) + ws, err := NewSandbox("", exampleProgram, "", false, false) if err != nil { t.Fatal(err) } diff --git a/internal/lsp/fake/sandbox.go b/internal/lsp/fake/sandbox.go index 2c56abd231..f41c454df8 100644 --- a/internal/lsp/fake/sandbox.go +++ b/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. diff --git a/internal/lsp/lsprpc/lsprpc_test.go b/internal/lsp/lsprpc/lsprpc_test.go index d7c2ee2986..1f0c626419 100644 --- a/internal/lsp/lsprpc/lsprpc_test.go +++ b/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) } diff --git a/internal/lsp/regtest/reg_test.go b/internal/lsp/regtest/reg_test.go index 9f0e28367a..0837faea50 100644 --- a/internal/lsp/regtest/reg_test.go +++ b/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) } diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go index aa5f665d93..ba425987c8 100644 --- a/internal/lsp/regtest/runner.go +++ b/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()) } }