Hyang-Ah Hana Kim (Gerrit)

unread,

Jun 24, 2026, 6:47:59 PM (3 days ago) Jun 24

to goph...@pubsubhelper.golang.org, Hyang-Ah Hana Kim, golang-co...@googlegroups.com

Hyang-Ah Hana Kim has uploaded the change for review

Commit message

gopls/internal/test/integration: respect empty mode filter in runner

Fix a bug in the integration test runner where specifying Modes(0)
(an empty mode filter) was treated as "unset" due to a zero-value
check, causing the runner to fall back to default modes.

This CL fixes this by changing the runner's modes configuration to a
pointer, allowing us to distinguish between "unset" (nil) and
"explicitly empty" (0).

This bug caused TestSimultaneousEdits to occasionally deadlock on
builders running with -short.

Under -short, DefaultModes() returns only Default mode (1), as defined
in regtest.go. TestSimultaneousEdits restricts its execution to
  Modes(DefaultModes() & (Forwarded|SeparateProcess))

The intention was to run the test only in forwarded modes.
However, DefaultModes() = 1, Forwarded=2 and SeparateProcess=4,
so the resultng mode filter always evaluates to 0 (empty).

Due to the fallback bug, the runner saw 0 as "unset" and
incorrectly fell back to running the test in Default mode
(value 1) anyway, triggering a deadlock on the unbuffered net.Pipe
transport.

Also add a regression test in the misc package to ensure Modes(0) is
respected and does not fall back.

For golang/go#76713

Change-Id: Ia1b06a9eedf7ad4d9cab424a3f88a5bcb5ad1063

Change diff

diff --git a/gopls/internal/test/integration/misc/runner_test.go b/gopls/internal/test/integration/misc/runner_test.go
new file mode 100644
index 0000000..92268ab
--- /dev/null
+++ b/gopls/internal/test/integration/misc/runner_test.go
@@ -0,0 +1,22 @@
+// Copyright 2026 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package misc
+
+import (
+	"testing"
+
+	. "golang.org/x/tools/gopls/internal/test/integration"
+)
+
+func TestModesZero(t *testing.T) {
+	called := false
+	WithOptions(Modes(0)).Run(t, "", func(t *testing.T, env *Env) {
+		called = true
+	})
+
+	if called {
+		t.Errorf("test function was called even though Modes(0) was specified")
+	}
+}
diff --git a/gopls/internal/test/integration/options.go b/gopls/internal/test/integration/options.go
index 176a8a6..28dd52b 100644
--- a/gopls/internal/test/integration/options.go
+++ b/gopls/internal/test/integration/options.go
@@ -18,7 +18,7 @@
 type runConfig struct {
 	editor        fake.EditorConfig
 	sandbox       fake.SandboxConfig
-	modes         Mode
+	modes         *Mode
 	noLogsOnError bool
 	writeGoSum    []string
 }
@@ -71,10 +71,10 @@
 // modes.
 func Modes(modes Mode) RunOption {
 	return optionSetter(func(opts *runConfig) {
-		if opts.modes != 0 {
+		if opts.modes != nil {
 			panic("modes set more than once")
 		}
-		opts.modes = modes
+		opts.modes = &modes
 	})
 }
 
diff --git a/gopls/internal/test/integration/runner.go b/gopls/internal/test/integration/runner.go
index ae77ed1..95b4c01 100644
--- a/gopls/internal/test/integration/runner.go
+++ b/gopls/internal/test/integration/runner.go
@@ -146,8 +146,8 @@
 			opt.set(&config)
 		}
 		modes := r.DefaultModes
-		if config.modes != 0 {
-			modes = config.modes
+		if config.modes != nil {
+			modes = *config.modes
 		}
 		if modes&tc.mode == 0 {
 			continue

Change information

Files:

A gopls/internal/test/integration/misc/runner_test.go
M gopls/internal/test/integration/options.go
M gopls/internal/test/integration/runner.go

Change size: S

Delta: 3 files changed, 27 insertions(+), 5 deletions(-)

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Hyang-Ah Hana Kim (Gerrit)

unread,

Jun 24, 2026, 7:26:59 PM (2 days ago) Jun 24

to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Hyang-Ah Hana Kim uploaded new patchset

Hyang-Ah Hana Kim uploaded patch set #2 to this change.

Following approvals got outdated and were removed:

TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com

Open in Gerrit

Related details

Attention set is empty

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alex Putman (Gerrit)

unread,

Jun 25, 2026, 6:03:03 PM (2 days ago) Jun 25

to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com

Attention needed from Hyang-Ah Hana Kim

Alex Putman added 2 comments

File gopls/internal/test/integration/misc/runner_test.go

Line 13, Patchset 2 (Latest):func TestRunnerModes(t *testing.T) {

Alex Putman . unresolved

Should unset Mode be tested? I can't tell if its guaranteed that existing tests cover it

File gopls/internal/test/integration/runner.go

Line 55, Patchset 2 (Latest):

type Mode int

const (
	// Default mode runs gopls with the default options, communicating over pipes
	// to emulate the lsp sidecar execution mode, which communicates over
	// stdin/stdout.
	//
	// It uses separate servers for each test, but a shared cache, to avoid
	// duplicating work when processing GOROOT.
	Default Mode = 1 << iota

Alex Putman . unresolved

It seems like Mode = 0 is a known state now. Maybe `Empty Mode = 0`? Or `NoMode`?

Open in Gerrit

Related details

Attention is currently required from:

Hyang-Ah Hana Kim

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Hyang-Ah Hana Kim (Gerrit)

unread,

Jun 26, 2026, 12:49:16 PM (18 hours ago) Jun 26

to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Attention needed from Hyang-Ah Hana Kim

Hyang-Ah Hana Kim uploaded new patchset

Hyang-Ah Hana Kim uploaded patch set #3 to this change.

Following approvals got outdated and were removed:

TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com

Open in Gerrit

Related details

Attention is currently required from:

Hyang-Ah Hana Kim

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

open

diffy

Hyang-Ah Hana Kim (Gerrit)

unread,

Jun 26, 2026, 12:50:46 PM (18 hours ago) Jun 26

to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, Alex Putman, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com

Attention needed from Alex Putman

Hyang-Ah Hana Kim added 2 comments

File gopls/internal/test/integration/misc/runner_test.go

Line 13, Patchset 2:func TestRunnerModes(t *testing.T) {

Alex Putman . resolved

Should unset Mode be tested? I can't tell if its guaranteed that existing tests cover it

Hyang-Ah Hana Kim

Done. Added unset mode test.

File gopls/internal/test/integration/runner.go

Line 55, Patchset 2:type Mode int



const (
	// Default mode runs gopls with the default options, communicating over pipes
	// to emulate the lsp sidecar execution mode, which communicates over
	// stdin/stdout.
	//
	// It uses separate servers for each test, but a shared cache, to avoid
	// duplicating work when processing GOROOT.
	Default Mode = 1 << iota

Alex Putman . resolved

It seems like Mode = 0 is a known state now. Maybe `Empty Mode = 0`? Or `NoMode`?

Hyang-Ah Hana Kim

Great idea. Added NoMode.

Open in Gerrit

Related details

Attention is currently required from:

Alex Putman

Submit Requirements:

Code-Review

No-Unresolved-Comments

Review-Enforcement
TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

unsatisfied_requirement

satisfied_requirement

open

diffy

Alex Putman (Gerrit)

unread,

Jun 26, 2026, 1:09:20 PM (18 hours ago) Jun 26

to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, golang...@luci-project-accounts.iam.gserviceaccount.com, golang-co...@googlegroups.com

Attention needed from Hyang-Ah Hana Kim

Alex Putman voted Code-Review+2

Code-Review

+2

Open in Gerrit

Related details

Attention is currently required from:

Hyang-Ah Hana Kim

Submit Requirements:

Code-Review
No-Unresolved-Comments
Review-Enforcement

TryBots-Pass

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Hyang-Ah Hana Kim (Gerrit)

unread,

Jun 26, 2026, 6:33:22 PM (12 hours ago) Jun 26

to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, golang...@luci-project-accounts.iam.gserviceaccount.com, Alex Putman, golang-co...@googlegroups.com

Hyang-Ah Hana Kim submitted the change

Change information

Commit message:

gopls/internal/test/integration: respect empty mode filter in runner

Fix a bug in the integration test runner where specifying Modes(0)
(an empty mode filter) was treated as "unset" due to a zero-value
check, causing the runner to fall back to default modes.

This CL fixes this by changing the runner's modes configuration to a
pointer, allowing us to distinguish between "unset" (nil) and

"explicitly empty" (0, NoMode).



This bug caused TestSimultaneousEdits to occasionally deadlock on
builders running with -short.

Under -short, DefaultModes() returns only Default mode (1), as defined
in regtest.go. TestSimultaneousEdits restricts its execution to
  Modes(DefaultModes() & (Forwarded|SeparateProcess))

The intention was to run the test only in forwarded modes.
However, DefaultModes() = 1, Forwarded=2 and SeparateProcess=4,
so the resultng mode filter always evaluates to 0 (empty).

Due to the fallback bug, the runner saw 0 as "unset" and
incorrectly fell back to running the test in Default mode
(value 1) anyway, triggering a deadlock on the unbuffered net.Pipe
transport.

Also add a regression test in the misc package to ensure Modes(0) is
respected and does not fall back.

For golang/go#76713

Change-Id: Ia1b06a9eedf7ad4d9cab424a3f88a5bcb5ad1063

Reviewed-on: https://go-review.googlesource.com/c/tools/+/794160

Reviewed-by: Alex Putman <apu...@golang.org>

LUCI-TryBot-Result: golang...@luci-project-accounts.iam.gserviceaccount.com <golang...@luci-project-accounts.iam.gserviceaccount.com>

Files:

A gopls/internal/test/integration/misc/runner_test.go
M gopls/internal/test/integration/options.go
M gopls/internal/test/integration/runner.go

Change size: M

Delta: 3 files changed, 87 insertions(+), 8 deletions(-)

Branch: refs/heads/master

Submit Requirements:

Code-Review: +2 by Alex Putman
TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[tools] gopls/internal/test/integration: respect empty mode filter in runner

Hyang-Ah Hana Kim (Gerrit)

Hyang-Ah Hana Kim has uploaded the change for review

Commit message

Change diff

Change information

Related details

Hyang-Ah Hana Kim (Gerrit)

Hyang-Ah Hana Kim uploaded new patchset

Related details

Alex Putman (Gerrit)

Alex Putman added 2 comments

Related details

Hyang-Ah Hana Kim (Gerrit)

Hyang-Ah Hana Kim uploaded new patchset

Related details

Hyang-Ah Hana Kim (Gerrit)

Hyang-Ah Hana Kim added 2 comments

Related details

Alex Putman (Gerrit)

Alex Putman voted Code-Review+2

Related details

Hyang-Ah Hana Kim (Gerrit)

Hyang-Ah Hana Kim submitted the change

Change information