gopls: fix a couple places where temporary files are not removed

Fix two places where gopls fails to clean up temporary files:
- In telemetry tests, the deferred cleanup was not run after os.Exit.
- In the GC details codelens, a persistent GC details directory is
  assumed; just use a temp directory instead.

Change-Id: Icef5a4b612ac1727fee7d2c65e99a90f73123081
Reviewed-on: https://go-review.googlesource.com/c/tools/+/579755
Reviewed-by: Alan Donovan <[email protected]>
Auto-Submit: Robert Findley <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: findleyr

gopls/internal/golang/gc_annotations.go

@@ -17,6 +17,7 @@ import (
 	"golang.org/x/tools/gopls/internal/cache/metadata"
 	"golang.org/x/tools/gopls/internal/protocol"
 	"golang.org/x/tools/gopls/internal/settings"
+	"golang.org/x/tools/internal/event"
 	"golang.org/x/tools/internal/gocommand"
 )
 
@@ -25,11 +26,16 @@ func GCOptimizationDetails(ctx context.Context, snapshot *cache.Snapshot, mp *me
 		return nil, nil
 	}
 	pkgDir := filepath.Dir(mp.CompiledGoFiles[0].Path())
-	outDir := filepath.Join(os.TempDir(), fmt.Sprintf("gopls-%d.details", os.Getpid()))
-
-	if err := os.MkdirAll(outDir, 0700); err != nil {
+	outDir, err := os.MkdirTemp("", fmt.Sprintf("gopls-%d.details", os.Getpid()))
+	if err != nil {
 		return nil, err
 	}
+	defer func() {
+		if err := os.RemoveAll(outDir); err != nil {
+			event.Error(ctx, "cleaning gcdetails dir", err)
+		}
+	}()
+
 	tmpFile, err := os.CreateTemp(os.TempDir(), "gopls-x")
 	if err != nil {
 		return nil, err

gopls/internal/telemetry/telemetry_test.go

@@ -31,8 +31,9 @@ func TestMain(m *testing.M) {
 		panic(err)
 	}
 	countertest.Open(tmp)
-	defer os.RemoveAll(tmp)
-	Main(m)
+	code := Main(m)
+	os.RemoveAll(tmp)
+	os.Exit(code)
 }
 
 func TestTelemetry(t *testing.T) {

gopls/internal/test/integration/codelens/codelens_test.go

@@ -6,6 +6,7 @@ package codelens
 
 import (
 	"fmt"
+	"os"
 	"testing"
 
 	"golang.org/x/tools/gopls/internal/server"
@@ -20,7 +21,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	Main(m)
+	os.Exit(Main(m))
 }
 
 func TestDisablingCodeLens(t *testing.T) {

gopls/internal/test/integration/completion/completion_test.go

@@ -6,6 +6,7 @@ package completion
 
 import (
 	"fmt"
+	"os"
 	"sort"
 	"strings"
 	"testing"
@@ -24,7 +25,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	Main(m)
+	os.Exit(Main(m))
 }
 
 const proxy = `

gopls/internal/test/integration/debug/debug_test.go

@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 	"io"
 	"net/http"
+	"os"
 	"strings"
 	"testing"
 
@@ -19,7 +20,7 @@ import (
 )
 
 func TestMain(m *testing.M) {
-	Main(m)
+	os.Exit(Main(m))
 }
 
 func TestBugNotification(t *testing.T) {

gopls/internal/test/integration/diagnostics/diagnostics_test.go

@@ -7,6 +7,7 @@ package diagnostics
 import (
 	"context"
 	"fmt"
+	"os"
 	"os/exec"
 	"testing"
 
@@ -20,7 +21,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	Main(m)
+	os.Exit(Main(m))
 }
 
 // Use mod.com for all go.mod files due to golang/go#35230.

gopls/internal/test/integration/doc.go

@@ -33,7 +33,7 @@
 //	)
 //
 //	func TestMain(m *testing.M) {
-//		Main(m, hooks.Options)
+//		os.Exit(Main(m, hooks.Options))
 //	}
 //
 // # Writing a simple integration test

gopls/internal/test/integration/inlayhints/inlayhints_test.go

@@ -4,6 +4,7 @@
 package inlayhint
 
 import (
+	"os"
 	"testing"
 
 	"golang.org/x/tools/gopls/internal/golang"
@@ -13,7 +14,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	Main(m)
+	os.Exit(Main(m))
 }
 
 func TestEnablingInlayHints(t *testing.T) {

gopls/internal/test/integration/misc/misc_test.go

@@ -5,6 +5,7 @@
 package misc
 
 import (
+	"os"
 	"strings"
 	"testing"
 
@@ -16,7 +17,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	integration.Main(m)
+	os.Exit(integration.Main(m))
 }
 
 // TestDocumentURIFix ensures that a DocumentURI supplied by the

gopls/internal/test/integration/modfile/modfile_test.go

@@ -5,6 +5,7 @@
 package modfile
 
 import (
+	"os"
 	"path/filepath"
 	"runtime"
 	"strings"
@@ -19,7 +20,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	Main(m)
+	os.Exit(Main(m))
 }
 
 const workspaceProxy = `

gopls/internal/test/integration/regtest.go

@@ -111,7 +111,24 @@ func DefaultModes() Mode {
 var runFromMain = false // true if Main has been called
 
 // Main sets up and tears down the shared integration test state.
-func Main(m *testing.M) {
+func Main(m *testing.M) (code int) {
+	defer func() {
+		if runner != nil {
+			if err := runner.Close(); err != nil {
+				fmt.Fprintf(os.Stderr, "closing test runner: %v\n", err)
+				// Cleanup is broken in go1.12 and earlier, and sometimes flakes on
+				// Windows due to file locking, but this is OK for our CI.
+				//
+				// Fail on go1.13+, except for windows and android which have shutdown problems.
+				if testenv.Go1Point() >= 13 && runtime.GOOS != "windows" && runtime.GOOS != "android" {
+					if code == 0 {
+						code = 1
+					}
+				}
+			}
+		}
+	}()
+
 	runFromMain = true
 
 	// golang/go#54461: enable additional debugging around hanging Go commands.
@@ -121,12 +138,12 @@ func Main(m *testing.M) {
 	// suite. See the documentation for runTestAsGoplsEnvvar for more details.
 	if os.Getenv(runTestAsGoplsEnvvar) == "true" {
 		tool.Main(context.Background(), cmd.New(), os.Args[1:])
-		os.Exit(0)
+		return 0
 	}
 
 	if !testenv.HasExec() {
 		fmt.Printf("skipping all tests: exec not supported on %s/%s\n", runtime.GOOS, runtime.GOARCH)
-		os.Exit(0)
+		return 0
 	}
 	testenv.ExitIfSmallMachine()
 
@@ -137,12 +154,12 @@ func Main(m *testing.M) {
 
 	if skipReason := checkBuilder(); skipReason != "" {
 		fmt.Printf("Skipping all tests: %s\n", skipReason)
-		os.Exit(0)
+		return 0
 	}
 
 	if err := testenv.HasTool("go"); err != nil {
 		fmt.Println("Missing go command")
-		os.Exit(1)
+		return 1
 	}
 
 	runner = &Runner{
@@ -168,19 +185,5 @@ func Main(m *testing.M) {
 	}
 	runner.tempDir = dir
 
-	var code int
-	defer func() {
-		if err := runner.Close(); err != nil {
-			fmt.Fprintf(os.Stderr, "closing test runner: %v\n", err)
-			// Cleanup is broken in go1.12 and earlier, and sometimes flakes on
-			// Windows due to file locking, but this is OK for our CI.
-			//
-			// Fail on go1.13+, except for windows and android which have shutdown problems.
-			if testenv.Go1Point() >= 13 && runtime.GOOS != "windows" && runtime.GOOS != "android" {
-				os.Exit(1)
-			}
-		}
-		os.Exit(code)
-	}()
-	code = m.Run()
+	return m.Run()
 }

gopls/internal/test/integration/template/template_test.go

@@ -5,6 +5,7 @@
 package template
 
 import (
+	"os"
 	"strings"
 	"testing"
 
@@ -15,7 +16,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	Main(m)
+	os.Exit(Main(m))
 }
 
 func TestMultilineTokens(t *testing.T) {

gopls/internal/test/integration/watch/watch_test.go

@@ -5,6 +5,7 @@
 package watch
 
 import (
+	"os"
 	"testing"
 
 	. "golang.org/x/tools/gopls/internal/test/integration"
@@ -16,7 +17,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	Main(m)
+	os.Exit(Main(m))
 }
 
 func TestEditFile(t *testing.T) {

gopls/internal/test/integration/workspace/workspace_test.go

@@ -7,6 +7,7 @@ package workspace
 import (
 	"context"
 	"fmt"
+	"os"
 	"sort"
 	"strings"
 	"testing"
@@ -24,7 +25,7 @@ import (
 
 func TestMain(m *testing.M) {
 	bug.PanicOnBugs = true
-	Main(m)
+	os.Exit(Main(m))
 }
 
 const workspaceProxy = `