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>
diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go
index 9cbf2f4..61d4ae2 100644
--- a/gopls/internal/regtest/bench/bench_test.go
+++ b/gopls/internal/regtest/bench/bench_test.go
@@ -10,7 +10,6 @@
"os"
"runtime/pprof"
"testing"
- "time"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/internal/lsp/fake"
@@ -32,9 +31,9 @@
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"),
diff --git a/gopls/internal/regtest/codelens/codelens_test.go b/gopls/internal/regtest/codelens/codelens_test.go
index 38721fb..3e15271 100644
--- a/gopls/internal/regtest/codelens/codelens_test.go
+++ b/gopls/internal/regtest/codelens/codelens_test.go
@@ -9,7 +9,6 @@
"runtime"
"strings"
"testing"
- "time"
"golang.org/x/tools/gopls/internal/hooks"
. "golang.org/x/tools/internal/lsp/regtest"
@@ -292,13 +291,6 @@
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 @@
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)
diff --git a/internal/lsp/regtest/regtest.go b/internal/lsp/regtest/regtest.go
index c1df26d..3180623 100644
--- a/internal/lsp/regtest/regtest.go
+++ b/internal/lsp/regtest/regtest.go
@@ -23,11 +23,24 @@
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 {
diff --git a/internal/lsp/regtest/runner.go b/internal/lsp/regtest/runner.go
index 05867c4..822a5a3 100644
--- a/internal/lsp/regtest/runner.go
+++ b/internal/lsp/regtest/runner.go
@@ -29,6 +29,7 @@
"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 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 @@
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 @@
}
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)
