From 9caf3e37e6a6c1581e028166c8b2431168ce8ac8 Mon Sep 17 00:00:00 2001 From: Suzy Mueller Date: Thu, 22 Dec 2022 14:31:31 -0500 Subject: [PATCH] [release] src/goTest: do not open text document on didCreateFile The Test Explorer watches all _test.go files in the workspace in order to keep the list of tests updated in the test explorer ui. On branch switches, there may be many test files that are created, causing the test explorer to process these. Previously, part of the processing resulted in opening the file. This led to issues due to the interaction with the language server. This change updates the logic on file creation, to add the parent test item only, which will not require opening the document. Fixes golang/vscode-go#2570 Change-Id: I595f34daee257c85bafd3e5706176f73f891fdf1 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/458999 Reviewed-by: Hyang-Ah Hana Kim Run-TryBot: Suzy Mueller TryBot-Result: kokoro Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/462376 Run-TryBot: Hyang-Ah Hana Kim Reviewed-by: Robert Findley --- src/goDocumentSymbols.ts | 4 ++++ src/goTest/explore.ts | 7 +++++- src/goTest/resolve.ts | 30 ++++++++++++------------- test/integration/goTest.explore.test.ts | 30 ++++++++++++++++--------- 4 files changed, 45 insertions(+), 26 deletions(-) diff --git a/src/goDocumentSymbols.ts b/src/goDocumentSymbols.ts index 5809237e8d..1aa814e9f9 100644 --- a/src/goDocumentSymbols.ts +++ b/src/goDocumentSymbols.ts @@ -25,6 +25,10 @@ export class GoplsDocumentSymbolProvider implements vscode.DocumentSymbolProvide constructor(private readonly goCtx: GoExtensionContext, private includeImports?: boolean) {} public async provideDocumentSymbols(document: vscode.TextDocument): Promise { + // TODO(suzmue): consider providing an interface for providing document symbols that only requires + // the URI. Getting a TextDocument from a filename requires opening the file, which can lead to + // files being opened that were not requested by the user in order to get information that we just + // need the URI to access. if (typeof this.includeImports !== 'boolean') { const gotoSymbolConfig = getGoConfig(document.uri)['gotoSymbol']; this.includeImports = gotoSymbolConfig ? gotoSymbolConfig['includeImports'] : false; diff --git a/src/goTest/explore.ts b/src/goTest/explore.ts index 5493b0e9c1..a9a75fe43f 100644 --- a/src/goTest/explore.ts +++ b/src/goTest/explore.ts @@ -258,7 +258,12 @@ export class GoTestExplorer { } protected async didCreateFile(file: Uri) { - await this.documentUpdate(await this.workspace.openTextDocument(file)); + // Do not use openTextDocument to get the TextDocument for file, + // since this sends a didOpen text document notification to gopls, + // leading to spurious diagnostics from gopls: + // https://github.com/golang/vscode-go/issues/2570 + // Instead, get the test item for this file only. + await this.resolver.getFile(file); } protected async didDeleteFile(file: Uri) { diff --git a/src/goTest/resolve.ts b/src/goTest/resolve.ts index 2b4d93c193..48c38bfde9 100644 --- a/src/goTest/resolve.ts +++ b/src/goTest/resolve.ts @@ -233,6 +233,21 @@ export class GoTestResolver { vscode.commands.executeCommand('setContext', 'go.tests', items); } + // Retrieve or create an item for a Go file. + public async getFile(uri: Uri): Promise { + const dir = path.dirname(uri.path); + const pkg = await this.getPackage(uri.with({ path: dir, query: '', fragment: '' })); + const existing = this.getItem(pkg, uri, 'file'); + if (existing) { + return existing; + } + + const label = path.basename(uri.path); + const item = this.getOrCreateItem(pkg, label, uri, 'file'); + item.canResolveChildren = true; + return item; + } + /* ***** Private ***** */ private shouldSetRange(item: TestItem): boolean { @@ -375,21 +390,6 @@ export class GoTestResolver { return item; } - // Retrieve or create an item for a Go file. - private async getFile(uri: Uri): Promise { - const dir = path.dirname(uri.path); - const pkg = await this.getPackage(uri.with({ path: dir, query: '', fragment: '' })); - const existing = this.getItem(pkg, uri, 'file'); - if (existing) { - return existing; - } - - const label = path.basename(uri.path); - const item = this.getOrCreateItem(pkg, label, uri, 'file'); - item.canResolveChildren = true; - return item; - } - private getTestSuite(type: string): TestSuite { if (this.testSuites.has(type)) { return this.testSuites.get(type) as TestSuite; diff --git a/test/integration/goTest.explore.test.ts b/test/integration/goTest.explore.test.ts index 0beda07255..ebef7b01fa 100644 --- a/test/integration/goTest.explore.test.ts +++ b/test/integration/goTest.explore.test.ts @@ -60,11 +60,16 @@ suite('Go Test Explorer', () => { async _didOpen(doc: TextDocument) { await this.didOpenTextDocument(doc); } + + async _didCreate(doc: TextDocument) { + await this.didCreateFile(doc.uri); + } } interface TC extends TestCase { - open: string; - expect: string[]; + uri: string; + expectCreate: string[]; + expectOpen: string[]; } const cases: Record = { @@ -76,8 +81,9 @@ suite('Go Test Explorer', () => { '/src/proj/bar_test.go': 'package main\nfunc TestBar(*testing.T) {}', '/src/proj/baz/main_test.go': 'package main\nfunc TestBaz(*testing.T) {}' }, - open: 'file:///src/proj/foo_test.go', - expect: [ + uri: 'file:///src/proj/foo_test.go', + expectCreate: ['file:///src/proj?module', 'file:///src/proj/foo_test.go?file'], + expectOpen: [ 'file:///src/proj?module', 'file:///src/proj/foo_test.go?file', 'file:///src/proj/foo_test.go?test#TestFoo' @@ -89,8 +95,9 @@ suite('Go Test Explorer', () => { '/src/proj/go.mod': 'module test', '/src/proj/foo_test.go': 'package main\nfunc TestFoo(*testing.T) {}' }, - open: 'file:///src/proj/foo_test.go', - expect: [ + uri: 'file:///src/proj/foo_test.go', + expectCreate: ['file:///src/proj?module', 'file:///src/proj/foo_test.go?file'], + expectOpen: [ 'file:///src/proj?module', 'file:///src/proj/foo_test.go?file', 'file:///src/proj/foo_test.go?test#TestFoo' @@ -100,14 +107,17 @@ suite('Go Test Explorer', () => { for (const name in cases) { test(name, async () => { - const { workspace, files, open, expect } = cases[name]; + const { workspace, files, uri: uri, expectCreate: expectCreate, expectOpen: expectOpen } = cases[name]; const { ctrl, expl, ws } = newExplorer(workspace, files, DUT); - const doc = ws.fs.files.get(open); + const doc = ws.fs.files.get(uri); assert(doc); - await expl._didOpen(doc); - assertTestItems(ctrl.items, expect); + await expl._didCreate(doc); + assertTestItems(ctrl.items, expectCreate); + + await expl._didOpen(doc); + assertTestItems(ctrl.items, expectOpen); }); } });