internal/lsp: improvements for command messages

When falling back to messages for progress reporting, don't try to
implement cancellation via ShowMessageCommand dialogs. They are an
imperfect solution, as the dialog stays open even after the command
completed. Also, among the LSP clients that don't support workDone
reporting, I suspect many also don't support ShowMessageCommand (for
example, govim), so the audience for this feature is probably quite
small.

Just remove it, and instead show a (non-cancellable) message. If clients
want cancellation, workDone progress support is the way to provide it.

Also remove a redundant message on go-generate success, and attach logs
when tests fail. Without logs on failure, I find that the test command
is not very useful. I tested a bit with very verbose test output, and
both VS Code and coc.nvim handled it gracefully.

Finally, fix a bug causing benchmarks not to be run.

Change-Id: I05422bcefc857c25cd99e643e614a0bc33870586
Reviewed-on: https://go-review.googlesource.com/c/tools/+/249702
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/lsp/command.go b/internal/lsp/command.go
index cbc9fa9..191af73 100644
--- a/internal/lsp/command.go
+++ b/internal/lsp/command.go
@@ -5,6 +5,7 @@
 package lsp
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -104,24 +105,24 @@
 	// clients are aware of the work item before the command completes. This
 	// matters for regtests, where having a continuous thread of work is
 	// convenient for assertions.
-	work := s.progress.start(ctx, title, "starting...", params.WorkDoneToken, cancel)
+	work := s.progress.start(ctx, title, title+": running...", params.WorkDoneToken, cancel)
 	go func() {
 		defer cancel()
 		err := s.runCommand(ctx, work, command, params.Arguments)
 		switch {
 		case errors.Is(err, context.Canceled):
-			work.end(command.Name + " canceled")
+			work.end(title + ": canceled")
 		case err != nil:
-			event.Error(ctx, fmt.Sprintf("%s: command error", command.Name), err)
-			work.end(command.Name + " failed")
+			event.Error(ctx, fmt.Sprintf("%s: command error", title), err)
+			work.end(title + ": failed")
 			// Show a message when work completes with error, because the progress end
 			// message is typically dismissed immediately by LSP clients.
 			s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
 				Type:    protocol.Error,
-				Message: fmt.Sprintf("An error occurred running %s: check the gopls logs.", command.Name),
+				Message: fmt.Sprintf("%s: An error occurred: %v", title, err),
 			})
 		default:
-			work.end(command.Name + " complete")
+			work.end(command.Name + ": completed")
 		}
 	}()
 	return nil, nil
@@ -233,19 +234,9 @@
 	pkgPath := pkgs[0].PkgPath()
 
 	// create output
+	buf := &bytes.Buffer{}
 	ew := &eventWriter{ctx: ctx, operation: "test"}
-	var title string
-	if len(tests) > 0 && len(benchmarks) > 0 {
-		title = "tests and benchmarks"
-	} else if len(tests) > 0 {
-		title = "tests"
-	} else if len(benchmarks) > 0 {
-		title = "benchmarks"
-	} else {
-		return errors.New("No functions were provided")
-	}
-
-	out := io.MultiWriter(ew, workDoneWriter{work})
+	out := io.MultiWriter(ew, workDoneWriter{work}, buf)
 
 	// Run `go test -run Func` on each test.
 	var failedTests int
@@ -262,7 +253,7 @@
 
 	// Run `go test -run=^$ -bench Func` on each test.
 	var failedBenchmarks int
-	for _, funcName := range tests {
+	for _, funcName := range benchmarks {
 		args := []string{pkgPath, "-v", "-run=^$", "-bench", fmt.Sprintf("^%s$", funcName)}
 		if err := snapshot.RunGoCommandPiped(ctx, "test", args, out, out); err != nil {
 			if errors.Is(err, context.Canceled) {
@@ -272,11 +263,17 @@
 		}
 	}
 
-	messageType := protocol.Info
-	message := fmt.Sprintf("all %s passed", title)
-	if failedTests > 0 || failedBenchmarks > 0 {
-		messageType = protocol.Error
+	var title string
+	if len(tests) > 0 && len(benchmarks) > 0 {
+		title = "tests and benchmarks"
+	} else if len(tests) > 0 {
+		title = "tests"
+	} else if len(benchmarks) > 0 {
+		title = "benchmarks"
+	} else {
+		return errors.New("No functions were provided")
 	}
+	message := fmt.Sprintf("all %s passed", title)
 	if failedTests > 0 && failedBenchmarks > 0 {
 		message = fmt.Sprintf("%d / %d tests failed and %d / %d benchmarks failed", failedTests, len(tests), failedBenchmarks, len(benchmarks))
 	} else if failedTests > 0 {
@@ -284,6 +281,11 @@
 	} else if failedBenchmarks > 0 {
 		message = fmt.Sprintf("%d / %d benchmarks failed", failedBenchmarks, len(benchmarks))
 	}
+	messageType := protocol.Info
+	if failedTests > 0 || failedBenchmarks > 0 {
+		messageType = protocol.Error
+		message += "\n" + buf.String()
+	}
 
 	return s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
 		Type:    messageType,
@@ -306,8 +308,5 @@
 	if err := snapshot.RunGoCommandPiped(ctx, "generate", args, er, stderr); err != nil {
 		return err
 	}
-	return s.client.ShowMessage(ctx, &protocol.ShowMessageParams{
-		Type:    protocol.Info,
-		Message: "go generate complete",
-	})
+	return nil
 }
diff --git a/internal/lsp/progress.go b/internal/lsp/progress.go
index cdaf172..d444ad1 100644
--- a/internal/lsp/progress.go
+++ b/internal/lsp/progress.go
@@ -66,7 +66,19 @@
 		cancel: cancel,
 	}
 	if !t.supportsWorkDoneProgress {
-		go wd.openStartMessage(message)
+		// Previous iterations of this fallback attempted to retain cancellation
+		// support by using ShowMessageCommand with a 'Cancel' button, but this is
+		// not ideal as the 'Cancel' dialog stays open even after the command
+		// completes.
+		//
+		// Just show a simple message. Clients can implement workDone progress
+		// reporting to get cancellation support.
+		if err := wd.client.ShowMessage(wd.ctx, &protocol.ShowMessageParams{
+			Type:    protocol.Log,
+			Message: message,
+		}); err != nil {
+			event.Error(ctx, "showing start message for "+title, err)
+		}
 		return wd
 	}
 	if wd.token == nil {
@@ -140,36 +152,6 @@
 	cleanup func()
 }
 
-func (wd *workDone) openStartMessage(msg string) {
-	go func() {
-		if wd.cancel == nil {
-			err := wd.client.ShowMessage(wd.ctx, &protocol.ShowMessageParams{
-				Type:    protocol.Log,
-				Message: msg,
-			})
-			if err != nil {
-				event.Error(wd.ctx, "error sending message request", err)
-			}
-			return
-		}
-		const cancel = "Cancel"
-		item, err := wd.client.ShowMessageRequest(wd.ctx, &protocol.ShowMessageRequestParams{
-			Type:    protocol.Log,
-			Message: msg,
-			Actions: []protocol.MessageActionItem{{
-				Title: cancel,
-			}},
-		})
-		if err != nil {
-			event.Error(wd.ctx, "error sending message request", err)
-			return
-		}
-		if item != nil && item.Title == cancel {
-			wd.doCancel()
-		}
-	}()
-}
-
 func (wd *workDone) doCancel() {
 	wd.cancelMu.Lock()
 	defer wd.cancelMu.Unlock()
diff --git a/internal/lsp/progress_test.go b/internal/lsp/progress_test.go
index 1a162a6..40ca3d2 100644
--- a/internal/lsp/progress_test.go
+++ b/internal/lsp/progress_test.go
@@ -9,7 +9,6 @@
 	"fmt"
 	"sync"
 	"testing"
-	"time"
 
 	"golang.org/x/tools/internal/lsp/protocol"
 )
@@ -21,8 +20,6 @@
 
 	mu                                        sync.Mutex
 	created, begun, reported, messages, ended int
-
-	cancel chan struct{}
 }
 
 func (c *fakeClient) checkToken(token protocol.ProgressToken) {
@@ -66,19 +63,8 @@
 	return nil
 }
 
-func (c *fakeClient) ShowMessageRequest(ctx context.Context, params *protocol.ShowMessageRequestParams) (*protocol.MessageActionItem, error) {
-	select {
-	case <-ctx.Done():
-	case <-c.cancel:
-		return &params.Actions[0], nil
-	}
-	return nil, nil
-}
-
 func setup(token protocol.ProgressToken) (context.Context, *progressTracker, *fakeClient) {
-	c := &fakeClient{
-		cancel: make(chan struct{}),
-	}
+	c := &fakeClient{}
 	tracker := newProgressTracker(c)
 	tracker.supportsWorkDoneProgress = true
 	return context.Background(), tracker, c
@@ -94,7 +80,7 @@
 	}{
 		{
 			name:         "unsupported",
-			wantMessages: 1,
+			wantMessages: 2,
 		},
 		{
 			name:         "random token",
@@ -173,24 +159,3 @@
 		}
 	}
 }
-
-func TestProgressTracker_MessageCancellation(t *testing.T) {
-	// Test that progress is canceled via the showMessageRequest dialog,
-	// when workDone is not supported.
-	ctx, tracker, client := setup(nil)
-	tracker.supportsWorkDoneProgress = false
-	ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
-	defer cancel()
-
-	canceled := make(chan struct{})
-	canceler := func() { close(canceled) }
-	tracker.start(ctx, "work", "message", nil, canceler)
-
-	close(client.cancel)
-
-	select {
-	case <-canceled:
-	case <-ctx.Done():
-		t.Errorf("timed out waiting for cancel")
-	}
-}
diff --git a/internal/lsp/source/command.go b/internal/lsp/source/command.go
index e92bbaa..fb178c7 100644
--- a/internal/lsp/source/command.go
+++ b/internal/lsp/source/command.go
@@ -59,37 +59,44 @@
 var (
 	// CommandTest runs `go test` for a specific test function.
 	CommandTest = &Command{
-		Name: "test",
+		Name:  "test",
+		Title: "Run test(s)",
 	}
 
 	// CommandGenerate runs `go generate` for a given directory.
 	CommandGenerate = &Command{
-		Name: "generate",
+		Name:  "generate",
+		Title: "Run go generate",
 	}
 
 	// CommandTidy runs `go mod tidy` for a module.
 	CommandTidy = &Command{
-		Name: "tidy",
+		Name:  "tidy",
+		Title: "Run go mod tidy",
 	}
 
 	// CommandVendor runs `go mod vendor` for a module.
 	CommandVendor = &Command{
-		Name: "vendor",
+		Name:  "vendor",
+		Title: "Run go mod vendor",
 	}
 
 	// CommandUpgradeDependency upgrades a dependency.
 	CommandUpgradeDependency = &Command{
-		Name: "upgrade_dependency",
+		Name:  "upgrade_dependency",
+		Title: "Upgrade dependency",
 	}
 
 	// CommandRegenerateCgo regenerates cgo definitions.
 	CommandRegenerateCgo = &Command{
-		Name: "regenerate_cgo",
+		Name:  "regenerate_cgo",
+		Title: "Regenerate cgo",
 	}
 
 	// CommandToggleDetails controls calculation of gc annotations.
 	CommandToggleDetails = &Command{
-		Name: "gc_details",
+		Name:  "gc_details",
+		Title: "Toggle gc_details",
 	}
 
 	// CommandFillStruct is a gopls command to fill a struct with default