internal/lsp/regtest: eliminate arbitrary timeouts

We care that gopls operations complete within a reasonable time.
However, what is “reasonable” depends strongly on the specifics of the
user and the hardware they are running on: a timeout that would be
perfectly reasonable on a high-powered user workstation with little
other load may be far too short on an overloaded and/or underpowered
CI builder.

This change adjusts the regtest runner to use the test deadline
instead of an arbitrary, flag-defined timeout; we expect the user or
system running the test to scale the test timeout appropriately to the
specific platform and system load.

When the testing package gains support for per-test timeouts
(golang/go#48157), this approach will automatically apply those
timeouts too.

If we decide that we also want to test specific performance and/or
latency targets, we can set up specific configurations for that (as
either aggressive per-test timeouts or benchmarks) in a followup
change.

For golang/go#50582

Change-Id: I1ab11b2049effb097aa620046fe11609269f91c4
Reviewed-on: https://go-review.googlesource.com/c/tools/+/380497
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>

Author: Bryan C. Mills

gopls/internal/regtest/bench/bench_test.go

@@ -10,7 +10,6 @@ import (
 	"os"
 	"runtime/pprof"
 	"testing"
-	"time"
 
 	"golang.org/x/tools/gopls/internal/hooks"
 	"golang.org/x/tools/internal/lsp/fake"
@@ -32,9 +31,9 @@ func benchmarkOptions(dir string) []RunOption {
 		SkipLogs(),
 		// The Debug server only makes sense if running in singleton mode.
 		Modes(Singleton),
-		// Set a generous timeout. Individual tests should control their own
-		// graceful termination.
-		Timeout(20 * time.Minute),
+		// Remove the default timeout. Individual tests should control their
+		// own graceful termination.
+		NoDefaultTimeout(),
 
 		// Use the actual proxy, since we want our builds to succeed.
 		GOPROXY("https://proxy.golang.org"),

gopls/internal/regtest/codelens/codelens_test.go

@@ -9,7 +9,6 @@ import (
 	"runtime"
 	"strings"
 	"testing"
-	"time"
 
 	"golang.org/x/tools/gopls/internal/hooks"
 	. "golang.org/x/tools/internal/lsp/regtest"
@@ -292,13 +291,6 @@ func TestGCDetails(t *testing.T) {
 		t.Skipf("the gc details code lens doesn't work on Android")
 	}
 
-	// TestGCDetails seems to suffer from poor performance on certain builders.
-	// Give it as long as it needs to complete.
-	timeout := 60 * time.Second
-	if d, ok := testenv.Deadline(t); ok {
-		timeout = time.Until(d) * 19 / 20 // Leave 5% headroom for cleanup.
-	}
-
 	const mod = `
 -- go.mod --
 module mod.com
@@ -318,7 +310,6 @@ func main() {
 			CodeLenses: map[string]bool{
 				"gc_details": true,
 			}},
-		Timeout(timeout),
 	).Run(t, mod, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		env.ExecuteCodeLensCommand("main.go", command.GCDetails)

internal/lsp/regtest/regtest.go

@@ -23,11 +23,24 @@ import (
 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", 20*time.Second, "default timeout for each regtest")
+	regtestTimeout           = flag.Duration("regtest_timeout", defaultRegtestTimeout(), "if nonzero, default timeout for each regtest; defaults to GOPLS_REGTEST_TIMEOUT")
 	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")
 )
 
+func defaultRegtestTimeout() time.Duration {
+	s := os.Getenv("GOPLS_REGTEST_TIMEOUT")
+	if s == "" {
+		return 0
+	}
+	d, err := time.ParseDuration(s)
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "invalid GOPLS_REGTEST_TIMEOUT %q: %v\n", s, err)
+		os.Exit(2)
+	}
+	return d
+}
+
 var runner *Runner
 
 type regtestRunner interface {

internal/lsp/regtest/runner.go

@@ -29,6 +29,7 @@ import (
 	"golang.org/x/tools/internal/lsp/lsprpc"
 	"golang.org/x/tools/internal/lsp/protocol"
 	"golang.org/x/tools/internal/lsp/source"
+	"golang.org/x/tools/internal/testenv"
 	"golang.org/x/tools/internal/xcontext"
 )
 
@@ -71,20 +72,19 @@ type Runner struct {
 }
 
 type runConfig struct {
-	editor      fake.EditorConfig
-	sandbox     fake.SandboxConfig
-	modes       Mode
-	timeout     time.Duration
-	debugAddr   string
-	skipLogs    bool
-	skipHooks   bool
-	optionsHook func(*source.Options)
+	editor           fake.EditorConfig
+	sandbox          fake.SandboxConfig
+	modes            Mode
+	noDefaultTimeout bool
+	debugAddr        string
+	skipLogs         bool
+	skipHooks        bool
+	optionsHook      func(*source.Options)
 }
 
 func (r *Runner) defaultConfig() *runConfig {
 	return &runConfig{
 		modes:       r.DefaultModes,
-		timeout:     r.Timeout,
 		optionsHook: r.OptionsHook,
 	}
 }
@@ -100,10 +100,12 @@ func (f optionSetter) set(opts *runConfig) {
 	f(opts)
 }
 
-// Timeout configures a custom timeout for this test run.
-func Timeout(d time.Duration) RunOption {
+// NoDefaultTimeout removes the timeout set by the -regtest_timeout flag, for
+// individual tests that are expected to run longer than is reasonable for
+// ordinary regression tests.
+func NoDefaultTimeout() RunOption {
 	return optionSetter(func(opts *runConfig) {
-		opts.timeout = d
+		opts.noDefaultTimeout = true
 	})
 }
 
@@ -257,8 +259,18 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio
 		}
 
 		t.Run(tc.name, func(t *testing.T) {
-			ctx, cancel := context.WithTimeout(context.Background(), config.timeout)
-			defer cancel()
+			ctx := context.Background()
+			if r.Timeout != 0 && !config.noDefaultTimeout {
+				var cancel context.CancelFunc
+				ctx, cancel = context.WithTimeout(ctx, r.Timeout)
+				defer cancel()
+			} else if d, ok := testenv.Deadline(t); ok {
+				timeout := time.Until(d) * 19 / 20 // Leave an arbitrary 5% for cleanup.
+				var cancel context.CancelFunc
+				ctx, cancel = context.WithTimeout(ctx, timeout)
+				defer cancel()
+			}
+
 			ctx = debug.WithInstance(ctx, "", "off")
 			if config.debugAddr != "" {
 				di := debug.GetInstance(ctx)