internal/lsp/regtest: only run /default tests with -short

Due to shared caching, the "default" tests can run faster than other
execution modes and still retain most of the test coverage. Update the
test runner to only run the singleton tests if testing.Short() is true,
independent of GOOS.

On the other hand, we lost noticeable coverage when disabling the
Forwarded testing mode. Now that the regtests are lighter weight in
general, reenable it on the longtests builders.

While at it, clean up tests that used the server-side Options hook to
instead use Settings, since clients would configure their server via
Settings and Options can't work with a shared daemon.

Updates golang/go#39384

Change-Id: I33e8b746188d795e88841727e6b7116cd4851dc2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/418774
Reviewed-by: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
gopls-CI: kokoro <[email protected]>

Author: findleyr

gopls/internal/regtest/completion/postfix_snippet_test.go

@@ -9,7 +9,6 @@ import (
 	"testing"
 
 	. "golang.org/x/tools/internal/lsp/regtest"
-	"golang.org/x/tools/internal/lsp/source"
 )
 
 func TestPostfixSnippetCompletion(t *testing.T) {
@@ -433,9 +432,11 @@ func foo() string {
 		},
 	}
 
-	r := WithOptions(Options(func(o *source.Options) {
-		o.ExperimentalPostfixCompletions = true
-	}))
+	r := WithOptions(
+		Settings{
+			"experimentalPostfixCompletions": true,
+		},
+	)
 	r.Run(t, mod, func(t *testing.T, env *Env) {
 		for _, c := range cases {
 			t.Run(c.name, func(t *testing.T) {

gopls/internal/regtest/misc/shared_test.go

@@ -24,7 +24,10 @@ func main() {
 	fmt.Println("Hello World.")
 }`
 
-func runShared(t *testing.T, testFunc func(env1 *Env, env2 *Env)) {
+// runShared is a helper to run a test in the same directory using both the
+// original env, and an additional other environment connected to the same
+// server.
+func runShared(t *testing.T, testFunc func(origEnv *Env, otherEnv *Env)) {
 	// Only run these tests in forwarded modes.
 	modes := DefaultModes() & (Forwarded | SeparateProcess)
 	WithOptions(Modes(modes)).Run(t, sharedProgram, func(t *testing.T, env1 *Env) {
@@ -38,28 +41,32 @@ func runShared(t *testing.T, testFunc func(env1 *Env, env2 *Env)) {
 }
 
 func TestSimultaneousEdits(t *testing.T) {
-	runShared(t, func(env1 *Env, env2 *Env) {
+	runShared(t, func(origEnv *Env, otherEnv *Env) {
 		// In editor #1, break fmt.Println as before.
-		env1.OpenFile("main.go")
-		env1.RegexpReplace("main.go", "Printl(n)", "")
+		origEnv.OpenFile("main.go")
+		origEnv.RegexpReplace("main.go", "Printl(n)", "")
 		// In editor #2 remove the closing brace.
-		env2.OpenFile("main.go")
-		env2.RegexpReplace("main.go", "\\)\n(})", "")
+		otherEnv.OpenFile("main.go")
+		otherEnv.RegexpReplace("main.go", "\\)\n(})", "")
 
 		// Now check that we got different diagnostics in each environment.
-		env1.Await(env1.DiagnosticAtRegexp("main.go", "Printl"))
-		env2.Await(env2.DiagnosticAtRegexp("main.go", "$"))
+		origEnv.Await(origEnv.DiagnosticAtRegexp("main.go", "Printl"))
+		otherEnv.Await(otherEnv.DiagnosticAtRegexp("main.go", "$"))
 	})
 }
 
 func TestShutdown(t *testing.T) {
-	runShared(t, func(env1 *Env, env2 *Env) {
-		if err := env1.Editor.Close(env1.Ctx); err != nil {
+	runShared(t, func(origEnv *Env, otherEnv *Env) {
+		// Close otherEnv, and verify that operation in the original environment is
+		// unaffected. Note: 'otherEnv' must be the environment being closed here.
+		// If we were to instead close 'env' here, we'd run into a duplicate
+		// shutdown when the test runner closes the original env.
+		if err := otherEnv.Editor.Close(otherEnv.Ctx); err != nil {
 			t.Errorf("closing first editor: %v", err)
 		}
 		// Now make an edit in editor #2 to trigger diagnostics.
-		env2.OpenFile("main.go")
-		env2.RegexpReplace("main.go", "\\)\n(})", "")
-		env2.Await(env2.DiagnosticAtRegexp("main.go", "$"))
+		origEnv.OpenFile("main.go")
+		origEnv.RegexpReplace("main.go", "\\)\n(})", "")
+		origEnv.Await(origEnv.DiagnosticAtRegexp("main.go", "$"))
 	})
 }

gopls/internal/regtest/workspace/workspace_test.go

@@ -15,7 +15,6 @@ import (
 	"golang.org/x/tools/internal/lsp/bug"
 	"golang.org/x/tools/internal/lsp/fake"
 	"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/lsp/regtest"
@@ -138,36 +137,22 @@ func TestReferences(t *testing.T) {
 	}
 }
 
-// make sure that directory filters work
-func TestFilters(t *testing.T) {
-	for _, tt := range []struct {
-		name, rootPath string
-	}{
-		{
-			name:     "module root",
-			rootPath: "pkg",
+func TestDirectoryFilters(t *testing.T) {
+	WithOptions(
+		ProxyFiles(workspaceProxy),
+		WorkspaceFolders("pkg"),
+		Settings{
+			"directoryFilters": []string{"-inner"},
 		},
-	} {
-		t.Run(tt.name, func(t *testing.T) {
-			opts := []RunOption{ProxyFiles(workspaceProxy)}
-			if tt.rootPath != "" {
-				opts = append(opts, WorkspaceFolders(tt.rootPath))
-			}
-			f := func(o *source.Options) {
-				o.DirectoryFilters = append(o.DirectoryFilters, "-inner")
+	).Run(t, workspaceModule, func(t *testing.T, env *Env) {
+		syms := env.WorkspaceSymbol("Hi")
+		sort.Slice(syms, func(i, j int) bool { return syms[i].ContainerName < syms[j].ContainerName })
+		for _, s := range syms {
+			if strings.Contains(s.ContainerName, "inner") {
+				t.Errorf("WorkspaceSymbol: found symbol %q with container %q, want \"inner\" excluded", s.Name, s.ContainerName)
 			}
-			opts = append(opts, Options(f))
-			WithOptions(opts...).Run(t, workspaceModule, func(t *testing.T, env *Env) {
-				syms := env.WorkspaceSymbol("Hi")
-				sort.Slice(syms, func(i, j int) bool { return syms[i].ContainerName < syms[j].ContainerName })
-				for i, s := range syms {
-					if strings.Contains(s.ContainerName, "/inner") {
-						t.Errorf("%s %v %s %s %d\n", s.Name, s.Kind, s.ContainerName, tt.name, i)
-					}
-				}
-			})
-		})
-	}
+		}
+	})
 }
 
 // Make sure that analysis diagnostics are cleared for the whole package when

internal/lsp/regtest/regtest.go

@@ -79,26 +79,17 @@ func (r RunMultiple) Run(t *testing.T, files string, f TestFunc) {
 	}
 }
 
-// The regtests run significantly slower on these operating systems, due to (we
-// believe) kernel locking behavior. Only run in singleton mode on these
-// operating system when using -short.
-var slowGOOS = map[string]bool{
-	"darwin":  true,
-	"openbsd": true,
-	"plan9":   true,
-}
-
+// DefaultModes returns the default modes to run for each regression test (they
+// may be reconfigured by the tests themselves).
 func DefaultModes() Mode {
-	// TODO(rfindley): these modes should *not* depend on GOOS. Depending on
-	// testing.Short() should be sufficient.
-	normal := Default | Experimental
-	if slowGOOS[runtime.GOOS] && testing.Short() {
-		normal = Default
+	modes := Default
+	if !testing.Short() {
+		modes |= Experimental | Forwarded
 	}
 	if *runSubprocessTests {
-		return normal | SeparateProcess
+		modes |= SeparateProcess
 	}
-	return normal
+	return modes
 }
 
 // Main sets up and tears down the shared regtest state.

internal/lsp/regtest/runner.go

@@ -85,6 +85,21 @@ const (
 	Experimental
 )
 
+func (m Mode) String() string {
+	switch m {
+	case Default:
+		return "default"
+	case Forwarded:
+		return "forwarded"
+	case SeparateProcess:
+		return "separate process"
+	case Experimental:
+		return "experimental"
+	default:
+		return "unknown mode"
+	}
+}
+
 // A Runner runs tests in gopls execution environments, as specified by its
 // modes. For modes that share state (for example, a shared cache or common
 // remote), any tests that execute on the same Runner will share the same
@@ -117,14 +132,6 @@ type runConfig struct {
 	debugAddr        string
 	skipLogs         bool
 	skipHooks        bool
-	optionsHook      func(*source.Options)
-}
-
-func (r *Runner) defaultConfig() *runConfig {
-	return &runConfig{
-		modes:       r.DefaultModes,
-		optionsHook: r.OptionsHook,
-	}
 }
 
 // A RunOption augments the behavior of the test runner.
@@ -155,22 +162,16 @@ func ProxyFiles(txt string) RunOption {
 }
 
 // Modes configures the execution modes that the test should run in.
+//
+// By default, modes are configured by the test runner. If this option is set,
+// it overrides the set of default modes and the test runs in exactly these
+// modes.
 func Modes(modes Mode) RunOption {
 	return optionSetter(func(opts *runConfig) {
-		opts.modes = modes
-	})
-}
-
-// Options configures the various server and user options.
-func Options(hook func(*source.Options)) RunOption {
-	return optionSetter(func(opts *runConfig) {
-		old := opts.optionsHook
-		opts.optionsHook = func(o *source.Options) {
-			if old != nil {
-				old(o)
-			}
-			hook(o)
+		if opts.modes != 0 {
+			panic("modes set more than once")
 		}
+		opts.modes = modes
 	})
 }
 
@@ -301,13 +302,18 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio
 
 	for _, tc := range tests {
 		tc := tc
-		config := r.defaultConfig()
+		var config runConfig
 		for _, opt := range opts {
-			opt.set(config)
+			opt.set(&config)
 		}
-		if config.modes&tc.mode == 0 {
+		modes := r.DefaultModes
+		if config.modes != 0 {
+			modes = config.modes
+		}
+		if modes&tc.mode == 0 {
 			continue
 		}
+
 		if config.debugAddr != "" && tc.mode != Default {
 			// Debugging is useful for running stress tests, but since the daemon has
 			// likely already been started, it would be too late to debug.
@@ -364,7 +370,9 @@ func (r *Runner) Run(t *testing.T, files string, test TestFunc, opts ...RunOptio
 					}
 				}
 			}()
-			ss := tc.getServer(t, config.optionsHook)
+
+			ss := tc.getServer(t, r.OptionsHook)
+
 			framer := jsonrpc2.NewRawStream
 			ls := &loggingFramer{}
 			if !config.skipLogs {