From ed71c572062891a960fad802d031c3558c5b1d03 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 17 Aug 2020 22:50:34 -0400 Subject: [PATCH] internal/lsp: consolidate progress reporting This change contains several improvements for progress reporting: + Consolidate the 'progressWriter' interface into the workDone interface. Now all progress reporting should use workDone, and the workDoneWriter struct is just an io.Writer adapter on top of workDone. + Factor out the pattern of progress reporting, and use for all asynchronous commands. + Make several commands that were previously synchronous async. + Add a test for cancellation when the WorkDone API is not supported. + Always report workdone progress using a detached context. + Update 'run tests' to use the -v option, and merge stderr and stdout, to increase the amount of information reported. + Since $/progress reporting is now always run with a detached context, the 'NoOutstandingWork' expectation should now behave correctly. Use it in a few places. A follow-up CL will improve the messages reported on command completion. For golang/go#40634 Change-Id: I7401ae62f7ed22d76e558ccc046e981622a64b12 Reviewed-on: https://go-review.googlesource.com/c/tools/+/248918 Run-TryBot: Robert Findley TryBot-Result: Gobot Gobot Reviewed-by: Heschi Kreinick --- internal/lsp/command.go | 132 +++++++++-------- internal/lsp/general.go | 6 +- internal/lsp/progress.go | 206 ++++++++++++++------------ internal/lsp/progress_test.go | 96 ++++++++++-- internal/lsp/regtest/codelens_test.go | 8 +- internal/lsp/regtest/wrappers.go | 3 +- internal/lsp/text_synchronization.go | 2 +- 7 files changed, 274 insertions(+), 179 deletions(-) diff --git a/internal/lsp/command.go b/internal/lsp/command.go index 1e4c1268f4..cbc9fa9433 100644 --- a/internal/lsp/command.go +++ b/internal/lsp/command.go @@ -6,12 +6,13 @@ package lsp import ( "context" + "encoding/json" "fmt" "io" + "log" "path" "golang.org/x/tools/internal/event" - "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" "golang.org/x/tools/internal/span" @@ -94,47 +95,78 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom } return nil, nil } - // Default commands that don't have suggested fix functions. + title := command.Title + if title == "" { + title = command.Name + } + ctx, cancel := context.WithCancel(xcontext.Detach(ctx)) + // Start progress prior to spinning off a goroutine specifically so that + // 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) + go func() { + defer cancel() + err := s.runCommand(ctx, work, command, params.Arguments) + switch { + case errors.Is(err, context.Canceled): + work.end(command.Name + " canceled") + case err != nil: + event.Error(ctx, fmt.Sprintf("%s: command error", command.Name), err) + work.end(command.Name + " 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), + }) + default: + work.end(command.Name + " complete") + } + }() + return nil, nil +} + +func (s *Server) runCommand(ctx context.Context, work *workDone, command *source.Command, args []json.RawMessage) error { switch command { case source.CommandTest: var uri protocol.DocumentURI var tests, benchmarks []string - if err := source.UnmarshalArgs(params.Arguments, &uri, &tests, &benchmarks); err != nil { - return nil, err + if err := source.UnmarshalArgs(args, &uri, &tests, &benchmarks); err != nil { + return err } snapshot, _, ok, release, err := s.beginFileRequest(ctx, uri, source.UnknownKind) defer release() if !ok { - return nil, err + return err } - go s.runTests(ctx, snapshot, uri, params.WorkDoneToken, tests, benchmarks) + return s.runTests(ctx, snapshot, uri, work, tests, benchmarks) case source.CommandGenerate: var uri protocol.DocumentURI var recursive bool - if err := source.UnmarshalArgs(params.Arguments, &uri, &recursive); err != nil { - return nil, err + if err := source.UnmarshalArgs(args, &uri, &recursive); err != nil { + return err } snapshot, _, ok, release, err := s.beginFileRequest(ctx, uri, source.UnknownKind) defer release() if !ok { - return nil, err + return err } - go s.runGoGenerate(xcontext.Detach(ctx), snapshot, uri.SpanURI(), recursive, params.WorkDoneToken) + return s.runGoGenerate(ctx, snapshot, uri.SpanURI(), recursive, work) case source.CommandRegenerateCgo: var uri protocol.DocumentURI - if err := source.UnmarshalArgs(params.Arguments, &uri); err != nil { - return nil, err + if err := source.UnmarshalArgs(args, &uri); err != nil { + return err } mod := source.FileModification{ URI: uri.SpanURI(), Action: source.InvalidateMetadata, } - err := s.didModifyFiles(ctx, []source.FileModification{mod}, FromRegenerateCgo) - return nil, err + return s.didModifyFiles(ctx, []source.FileModification{mod}, FromRegenerateCgo) case source.CommandTidy, source.CommandVendor: var uri protocol.DocumentURI - if err := source.UnmarshalArgs(params.Arguments, &uri); err != nil { - return nil, err + if err := source.UnmarshalArgs(args, &uri); err != nil { + return err } // The flow for `go mod tidy` and `go mod vendor` is almost identical, // so we combine them into one case for convenience. @@ -142,20 +174,18 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom if command == source.CommandVendor { a = "vendor" } - err := s.directGoModCommand(ctx, uri, "mod", []string{a}...) - return nil, err + return s.directGoModCommand(ctx, uri, "mod", []string{a}...) case source.CommandUpgradeDependency: var uri protocol.DocumentURI var goCmdArgs []string - if err := source.UnmarshalArgs(params.Arguments, &uri, &goCmdArgs); err != nil { - return nil, err + if err := source.UnmarshalArgs(args, &uri, &goCmdArgs); err != nil { + return err } - err := s.directGoModCommand(ctx, uri, "get", goCmdArgs...) - return nil, err + return s.directGoModCommand(ctx, uri, "get", goCmdArgs...) case source.CommandToggleDetails: var fileURI span.URI - if err := source.UnmarshalArgs(params.Arguments, &fileURI); err != nil { - return nil, err + if err := source.UnmarshalArgs(args, &fileURI); err != nil { + return err } pkgDir := span.URIFromPath(path.Dir(fileURI.Filename())) s.gcOptimizationDetailsMu.Lock() @@ -171,16 +201,15 @@ func (s *Server) executeCommand(ctx context.Context, params *protocol.ExecuteCom // so find the snapshot sv, err := s.session.ViewOf(fileURI) if err != nil { - return nil, err + return err } snapshot, release := sv.Snapshot(ctx) defer release() s.diagnoseSnapshot(snapshot) - return nil, nil default: - return nil, fmt.Errorf("unknown command: %s", params.Command) + return fmt.Errorf("unsupported command: %s", command.Name) } - return nil, nil + return nil } func (s *Server) directGoModCommand(ctx context.Context, uri protocol.DocumentURI, verb string, args ...string) error { @@ -193,10 +222,7 @@ func (s *Server) directGoModCommand(ctx context.Context, uri protocol.DocumentUR return snapshot.RunGoCommandDirect(ctx, verb, args) } -func (s *Server) runTests(ctx context.Context, snapshot source.Snapshot, uri protocol.DocumentURI, token protocol.ProgressToken, tests, benchmarks []string) error { - ctx, cancel := context.WithCancel(ctx) - defer cancel() - +func (s *Server) runTests(ctx context.Context, snapshot source.Snapshot, uri protocol.DocumentURI, work *workDone, tests, benchmarks []string) error { pkgs, err := snapshot.PackagesForFile(ctx, uri.SpanURI(), source.TypecheckWorkspace) if err != nil { return err @@ -218,17 +244,15 @@ func (s *Server) runTests(ctx context.Context, snapshot source.Snapshot, uri pro } else { return errors.New("No functions were provided") } - msg := fmt.Sprintf("Running %s...", title) - wc := s.progress.newWriter(ctx, title, msg, msg, token, cancel) - defer wc.Close() - stderr := io.MultiWriter(ew, wc) + out := io.MultiWriter(ew, workDoneWriter{work}) - // run `go test -run Func` on each test + // Run `go test -run Func` on each test. var failedTests int for _, funcName := range tests { - args := []string{pkgPath, "-run", fmt.Sprintf("^%s$", funcName)} - if err := snapshot.RunGoCommandPiped(ctx, "test", args, ew, stderr); err != nil { + args := []string{pkgPath, "-v", "-count=1", "-run", fmt.Sprintf("^%s$", funcName)} + log.Printf("running with these args: %v", args) + if err := snapshot.RunGoCommandPiped(ctx, "test", args, out, out); err != nil { if errors.Is(err, context.Canceled) { return err } @@ -236,11 +260,11 @@ func (s *Server) runTests(ctx context.Context, snapshot source.Snapshot, uri pro } } - // run `go test -run=^$ -bench Func` on each test + // Run `go test -run=^$ -bench Func` on each test. var failedBenchmarks int for _, funcName := range tests { - args := []string{pkgPath, "-run=^$", "-bench", fmt.Sprintf("^%s$", funcName)} - if err := snapshot.RunGoCommandPiped(ctx, "test", args, ew, stderr); err != nil { + 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) { return err } @@ -267,33 +291,23 @@ func (s *Server) runTests(ctx context.Context, snapshot source.Snapshot, uri pro }) } -// GenerateWorkDoneTitle is the title used in progress reporting for go -// generate commands. It is exported for testing purposes. -const GenerateWorkDoneTitle = "generate" - -func (s *Server) runGoGenerate(ctx context.Context, snapshot source.Snapshot, uri span.URI, recursive bool, token protocol.ProgressToken) error { +func (s *Server) runGoGenerate(ctx context.Context, snapshot source.Snapshot, uri span.URI, recursive bool, work *workDone) error { ctx, cancel := context.WithCancel(ctx) defer cancel() er := &eventWriter{ctx: ctx, operation: "generate"} - wc := s.progress.newWriter(ctx, GenerateWorkDoneTitle, "running go generate", "started go generate, check logs for progress", token, cancel) - defer wc.Close() args := []string{"-x"} if recursive { args = append(args, "./...") } - stderr := io.MultiWriter(er, wc) + stderr := io.MultiWriter(er, workDoneWriter{work}) if err := snapshot.RunGoCommandPiped(ctx, "generate", args, er, stderr); err != nil { - if errors.Is(err, context.Canceled) { - return nil - } - event.Error(ctx, "generate: command error", err, tag.Directory.Of(uri.Filename())) - return s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ - Type: protocol.Error, - Message: "go generate exited with an error, check gopls logs", - }) + return err } - return nil + return s.client.ShowMessage(ctx, &protocol.ShowMessageParams{ + Type: protocol.Info, + Message: "go generate complete", + }) } diff --git a/internal/lsp/general.go b/internal/lsp/general.go index d499d8f315..1ddc993c8f 100644 --- a/internal/lsp/general.go +++ b/internal/lsp/general.go @@ -190,7 +190,7 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol defer func() { go func() { wg.Wait() - work.end(ctx, "Done.") + work.end("Done.") }() }() } @@ -201,12 +201,12 @@ func (s *Server) addFolders(ctx context.Context, folders []protocol.WorkspaceFol view, snapshot, release, err := s.addView(ctx, folder.Name, uri) if err != nil { viewErrors[uri] = err - work.end(ctx, fmt.Sprintf("Error loading packages: %s", err)) + work.end(fmt.Sprintf("Error loading packages: %s", err)) continue } go func() { view.AwaitInitialized(ctx) - work.end(ctx, "Finished loading packages.") + work.end("Finished loading packages.") }() for _, dir := range snapshot.WorkspaceDirectories(ctx) { diff --git a/internal/lsp/progress.go b/internal/lsp/progress.go index fc63fd2a58..cdaf172ed4 100644 --- a/internal/lsp/progress.go +++ b/internal/lsp/progress.go @@ -6,7 +6,6 @@ package lsp import ( "context" - "io" "math/rand" "strconv" "sync" @@ -14,6 +13,7 @@ import ( "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/protocol" + "golang.org/x/tools/internal/xcontext" errors "golang.org/x/xerrors" ) @@ -32,15 +32,16 @@ func newProgressTracker(client protocol.Client) *progressTracker { } } -// start issues a $/progress notification to begin a unit of work on the -// server. The returned WorkDone handle may be used to report incremental +// start notifies the client of work being done on the server. It uses either +// ShowMessage RPCs or $/progress messages, depending on the capabilities of +// the client. The returned WorkDone handle may be used to report incremental // progress, and to report work completion. In particular, it is an error to // call start and not call end(...) on the returned WorkDone handle. // // If token is empty, a token will be randomly generated. // // The progress item is considered cancellable if the given cancel func is -// non-nil. +// non-nil. In this case, cancel is called when the work done // // Example: // func Generate(ctx) (err error) { @@ -59,25 +60,29 @@ func newProgressTracker(client protocol.Client) *progressTracker { // func (t *progressTracker) start(ctx context.Context, title, message string, token protocol.ProgressToken, cancel func()) *workDone { wd := &workDone{ + ctx: xcontext.Detach(ctx), client: t.client, token: token, cancel: cancel, } if !t.supportsWorkDoneProgress { - wd.startErr = errors.New("workdone reporting is not supported") + go wd.openStartMessage(message) return wd } if wd.token == nil { - wd.token = strconv.FormatInt(rand.Int63(), 10) + token = strconv.FormatInt(rand.Int63(), 10) err := wd.client.WorkDoneProgressCreate(ctx, &protocol.WorkDoneProgressCreateParams{ - Token: wd.token, + Token: token, }) if err != nil { - wd.startErr = err + wd.err = err event.Error(ctx, "starting work for "+title, err) return wd } + wd.token = token } + // At this point we have a token that the client knows about. Store the token + // before starting work. t.mu.Lock() t.inProgress[wd.token] = wd t.mu.Unlock() @@ -111,38 +116,82 @@ func (t *progressTracker) cancel(ctx context.Context, token protocol.ProgressTok if wd.cancel == nil { return errors.Errorf("work %q is not cancellable", token) } - wd.cancel() + wd.doCancel() return nil } -// newProgressWriter returns an io.WriterCloser that can be used -// to report progress on a command based on the client capabilities. -func (t *progressTracker) newWriter(ctx context.Context, title, beginMsg, msg string, token protocol.ProgressToken, cancel func()) io.WriteCloser { - if t.supportsWorkDoneProgress { - wd := t.start(ctx, title, beginMsg, token, cancel) - return &workDoneWriter{ctx, wd} - } - mw := &messageWriter{ctx, cancel, t.client} - mw.start(msg) - return mw -} - // workDone represents a unit of work that is reported to the client via the // progress API. type workDone struct { - client protocol.Client - startErr error - token protocol.ProgressToken - cancel func() - cleanup func() + // ctx is detached, for sending $/progress updates. + ctx context.Context + client protocol.Client + // If token is nil, this workDone object uses the ShowMessage API, rather + // than $/progress. + token protocol.ProgressToken + // err is set if progress reporting is broken for some reason (for example, + // if there was an initial error creating a token). + err error + + cancelMu sync.Mutex + cancelled bool + cancel func() + + 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() + if !wd.cancelled { + wd.cancel() + } } // report reports an update on WorkDone report back to the client. -func (wd *workDone) report(ctx context.Context, message string, percentage float64) error { - if wd.startErr != nil { - return wd.startErr +func (wd *workDone) report(message string, percentage float64) { + wd.cancelMu.Lock() + cancelled := wd.cancelled + wd.cancelMu.Unlock() + if cancelled { + return } - return wd.client.Progress(ctx, &protocol.ProgressParams{ + if wd.err != nil || wd.token == nil { + // Not using the workDone API, so we do nothing. It would be far too spammy + // to send incremental messages. + return + } + err := wd.client.Progress(wd.ctx, &protocol.ProgressParams{ Token: wd.token, Value: &protocol.WorkDoneProgressReport{ Kind: "report", @@ -154,24 +203,38 @@ func (wd *workDone) report(ctx context.Context, message string, percentage float Percentage: percentage, }, }) + if err != nil { + event.Error(wd.ctx, "reporting progress", err) + } } // end reports a workdone completion back to the client. -func (wd *workDone) end(ctx context.Context, message string) error { - if wd.startErr != nil { - return wd.startErr - } - err := wd.client.Progress(ctx, &protocol.ProgressParams{ - Token: wd.token, - Value: &protocol.WorkDoneProgressEnd{ - Kind: "end", +func (wd *workDone) end(message string) { + var err error + switch { + case wd.err != nil: + // There is a prior error. + case wd.token == nil: + // We're falling back to message-based reporting. + err = wd.client.ShowMessage(wd.ctx, &protocol.ShowMessageParams{ + Type: protocol.Info, Message: message, - }, - }) + }) + default: + err = wd.client.Progress(wd.ctx, &protocol.ProgressParams{ + Token: wd.token, + Value: &protocol.WorkDoneProgressEnd{ + Kind: "end", + Message: message, + }, + }) + } + if err != nil { + event.Error(wd.ctx, "ending work", err) + } if wd.cleanup != nil { wd.cleanup() } - return err } // eventWriter writes every incoming []byte to @@ -187,61 +250,14 @@ func (ew *eventWriter) Write(p []byte) (n int, err error) { return len(p), nil } -// messageWriter implements progressWriter and only tells the user that -// a command has started through window/showMessage, but does not report -// anything afterwards. This is because each log shows up as a separate window -// and therefore would be obnoxious to show every incoming line. Request -// cancellation happens synchronously through the ShowMessageRequest response. -type messageWriter struct { - ctx context.Context - cancel func() - client protocol.Client +// workDoneWriter wraps a workDone handle to provide a Writer interface, +// so that workDone reporting can more easily be hooked into commands. +type workDoneWriter struct { + wd *workDone } -func (lw *messageWriter) Write(p []byte) (n int, err error) { +func (wdw workDoneWriter) Write(p []byte) (n int, err error) { + wdw.wd.report(string(p), 0) + // Don't fail just because of a failure to report progress. return len(p), nil } - -func (lw *messageWriter) start(msg string) { - go func() { - const cancel = "Cancel" - item, err := lw.client.ShowMessageRequest(lw.ctx, &protocol.ShowMessageRequestParams{ - Type: protocol.Log, - Message: msg, - Actions: []protocol.MessageActionItem{{ - Title: "Cancel", - }}, - }) - if err != nil { - event.Error(lw.ctx, "error sending message request", err) - return - } - if item != nil && item.Title == "Cancel" { - lw.cancel() - } - }() -} - -func (lw *messageWriter) Close() error { - return lw.client.ShowMessage(lw.ctx, &protocol.ShowMessageParams{ - Type: protocol.Info, - Message: "go generate has finished", - }) -} - -// workDoneWriter implements progressWriter by sending $/progress notifications -// to the client. Request cancellations happens separately through the -// window/workDoneProgress/cancel request, in which case the given context will -// be rendered done. -type workDoneWriter struct { - ctx context.Context - wd *workDone -} - -func (wdw *workDoneWriter) Write(p []byte) (n int, err error) { - return len(p), wdw.wd.report(wdw.ctx, string(p), 0) -} - -func (wdw *workDoneWriter) Close() error { - return wdw.wd.end(wdw.ctx, "finished") -} diff --git a/internal/lsp/progress_test.go b/internal/lsp/progress_test.go index 470127edc4..1a162a6322 100644 --- a/internal/lsp/progress_test.go +++ b/internal/lsp/progress_test.go @@ -7,7 +7,9 @@ package lsp import ( "context" "fmt" + "sync" "testing" + "time" "golang.org/x/tools/internal/lsp/protocol" ) @@ -17,7 +19,10 @@ type fakeClient struct { token protocol.ProgressToken - created, begun, reported, ended int + mu sync.Mutex + created, begun, reported, messages, ended int + + cancel chan struct{} } func (c *fakeClient) checkToken(token protocol.ProgressToken) { @@ -30,12 +35,16 @@ func (c *fakeClient) checkToken(token protocol.ProgressToken) { } func (c *fakeClient) WorkDoneProgressCreate(ctx context.Context, params *protocol.WorkDoneProgressCreateParams) error { + c.mu.Lock() + defer c.mu.Unlock() c.checkToken(params.Token) c.created++ return nil } func (c *fakeClient) Progress(ctx context.Context, params *protocol.ProgressParams) error { + c.mu.Lock() + defer c.mu.Unlock() c.checkToken(params.Token) switch params.Value.(type) { case *protocol.WorkDoneProgressBegin: @@ -50,8 +59,26 @@ func (c *fakeClient) Progress(ctx context.Context, params *protocol.ProgressPara return nil } +func (c *fakeClient) ShowMessage(context.Context, *protocol.ShowMessageParams) error { + c.mu.Lock() + defer c.mu.Unlock() + c.messages++ + return nil +} + +func (c *fakeClient) ShowMessageRequest(ctx context.Context, params *protocol.ShowMessageRequestParams) (*protocol.MessageActionItem, error) { + select { + case <-ctx.Done(): + case <-c.cancel: + return ¶ms.Actions[0], nil + } + return nil, nil +} + func setup(token protocol.ProgressToken) (context.Context, *progressTracker, *fakeClient) { - c := &fakeClient{} + c := &fakeClient{ + cancel: make(chan struct{}), + } tracker := newProgressTracker(c) tracker.supportsWorkDoneProgress = true return context.Background(), tracker, c @@ -63,9 +90,11 @@ func TestProgressTracker_Reporting(t *testing.T) { supported bool token protocol.ProgressToken wantReported, wantCreated, wantBegun, wantEnded int + wantMessages int }{ { - name: "unsupported", + name: "unsupported", + wantMessages: 1, }, { name: "random token", @@ -95,22 +124,36 @@ func TestProgressTracker_Reporting(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { ctx, tracker, client := setup(test.token) + ctx, cancel := context.WithCancel(ctx) + defer cancel() tracker.supportsWorkDoneProgress = test.supported work := tracker.start(ctx, "work", "message", test.token, nil) - if got := client.created; got != test.wantCreated { - t.Errorf("got %d created tokens, want %d", got, test.wantCreated) + client.mu.Lock() + gotCreated, gotBegun := client.created, client.begun + client.mu.Unlock() + if gotCreated != test.wantCreated { + t.Errorf("got %d created tokens, want %d", gotCreated, test.wantCreated) } - if got := client.begun; got != test.wantBegun { - t.Errorf("got %d work begun, want %d", got, test.wantBegun) + if gotBegun != test.wantBegun { + t.Errorf("got %d work begun, want %d", gotBegun, test.wantBegun) } // Ignore errors: this is just testing the reporting behavior. - work.report(ctx, "report", 50) - if got := client.reported; got != test.wantReported { - t.Errorf("got %d progress reports, want %d", got, test.wantCreated) + work.report("report", 50) + client.mu.Lock() + gotReported := client.reported + client.mu.Unlock() + if gotReported != test.wantReported { + t.Errorf("got %d progress reports, want %d", gotReported, test.wantCreated) } - work.end(ctx, "done") - if got := client.ended; got != test.wantEnded { - t.Errorf("got %d ended reports, want %d", got, test.wantEnded) + work.end("done") + client.mu.Lock() + gotEnded, gotMessages := client.ended, client.messages + client.mu.Unlock() + if gotEnded != test.wantEnded { + t.Errorf("got %d ended reports, want %d", gotEnded, test.wantEnded) + } + if gotMessages != test.wantMessages { + t.Errorf("got %d messages, want %d", gotMessages, test.wantMessages) } }) } @@ -119,14 +162,35 @@ func TestProgressTracker_Reporting(t *testing.T) { func TestProgressTracker_Cancellation(t *testing.T) { for _, token := range []protocol.ProgressToken{nil, 1, "a"} { ctx, tracker, _ := setup(token) - var cancelled bool - cancel := func() { cancelled = true } + var canceled bool + cancel := func() { canceled = true } work := tracker.start(ctx, "work", "message", token, cancel) if err := tracker.cancel(ctx, work.token); err != nil { t.Fatal(err) } - if !cancelled { + if !canceled { t.Errorf("tracker.cancel(...): cancel not called") } } } + +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/regtest/codelens_test.go b/internal/lsp/regtest/codelens_test.go index e5bf57ddac..cb9443aa9e 100644 --- a/internal/lsp/regtest/codelens_test.go +++ b/internal/lsp/regtest/codelens_test.go @@ -60,9 +60,9 @@ const ( } // This test confirms the full functionality of the code lenses for updating -// dependencies in a go.mod file. It checks for the code lens that suggests an -// update and then executes the command associated with that code lens. -// A regression test for golang/go#39446. +// dependencies in a go.mod file. It checks for the code lens that suggests +// an update and then executes the command associated with that code lens. A +// regression test for golang/go#39446. func TestUpdateCodelens(t *testing.T) { const proxyWithLatest = ` -- golang.org/x/hello@v1.3.3/go.mod -- @@ -119,6 +119,7 @@ func main() { }); err != nil { t.Fatal(err) } + env.Await(NoOutstandingWork()) got := env.ReadWorkspaceFile("go.mod") const wantGoMod = `module mod.com @@ -189,6 +190,7 @@ func main() { }); err != nil { t.Fatal(err) } + env.Await(NoOutstandingWork()) got := env.ReadWorkspaceFile("go.mod") const wantGoMod = `module mod.com diff --git a/internal/lsp/regtest/wrappers.go b/internal/lsp/regtest/wrappers.go index 89bc656a01..66e19f51fd 100644 --- a/internal/lsp/regtest/wrappers.go +++ b/internal/lsp/regtest/wrappers.go @@ -9,7 +9,6 @@ import ( "io" "testing" - "golang.org/x/tools/internal/lsp" "golang.org/x/tools/internal/lsp/fake" "golang.org/x/tools/internal/lsp/protocol" ) @@ -217,7 +216,7 @@ func (e *Env) RunGenerate(dir string) { if err := e.Editor.RunGenerate(e.Ctx, dir); err != nil { e.T.Fatal(err) } - e.Await(CompletedWork(lsp.GenerateWorkDoneTitle, 1)) + e.Await(NoOutstandingWork()) // Ideally the fake.Workspace would handle all synthetic file watching, but // we help it out here as we need to wait for the generate command to // complete before checking the filesystem. diff --git a/internal/lsp/text_synchronization.go b/internal/lsp/text_synchronization.go index c709a3a5e1..29e4ca98f8 100644 --- a/internal/lsp/text_synchronization.go +++ b/internal/lsp/text_synchronization.go @@ -185,7 +185,7 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File defer func() { go func() { diagnosticWG.Wait() - work.end(ctx, "Done.") + work.end("Done.") }() }() }