Test explorer: leaves open files in the LSP session following on-disk changes

[findleyr]

Just observed this behavior very reliably when testing branch switches in vscode-go:

When I switch branches, vscode-go sends a lot of didOpen notifications to gopls for test files (_test.go). This can lead to spurious "no packages for open file foo_test.go" diagnostics from gopls. Worse, switching branches again leaves the open file state, which can lead to type-checking errors as the open contents no longer compile.

Setting "go.testExplorer.enable": false reliably avoids these problems.

[hyangah]

I think this is one of the problematic logic -

vscode-go/src/goTest/explore.ts

Line 261 in 7df32c8

await this.documentUpdate(await this.workspace.openTextDocument(file));

When switching branches and noticing new files, vscode fires onDidCreateFile events.
openTextDocument loads the file in the buffer which isn't necessarily visible to users.
We shouldn't openTextDocument just because file creation events were detected.

I also observed that when branch switch causes folder move or deletion, the tests under deleted folder should be removed but don't since currently the code relies on onDidDeleteFile events to remove the corresponding test items from the tree, but vscode does not always trigger deletion events when folder deletion occurs outside of vscode (e.g. git change or rm from terminal).

[suzmue]

Relevant upstream discussion about extensions wanting to get file contents without sending didOpen notifications to language servers: microsoft/vscode#15723. The conclusion from that thread was that the API is fine as is and this is just the way the world works.

The interesting thing is that we don't get see the 'didOpen' notifications in the language server for non-test files even though we called openTextDocument on those as well. Maybe its because we aren't holding on to them for long enough or it may be some way that we use the TextDocument later in the code.

Edit: Right, never mind. The reason we are only seeing the didOpen is because the TestExplorer is only watching for test files.

[findleyr]

I think it's still a bug that the files remain open after being read, causing subsequent errors when switching branches.

Furthermore, a didOpen is not a no-op for gopls: the set of open files is significant to gopls (for example, to warn about orphaned files). If we could avoid the didOpen notifications altogether, that would be ideal.

[hyangah]

We don't need to read the file contents at all until the file is actually open by the user and vscode fires a didOpen notification.

When initializing the tests explorer runs documentUpdate code only for

• files that are already open (which is ok)
• the user expanded the file item (which will still openTextDocument which isn't ideal but damage is more contained).

Making the onDidCreateFile handler not attempt to inspect the file content is consistent with the behavior and I think that can be a short term solution to improve the stability.

It looks like Test explorer needs the contents because it needs to call DocumentSymbol provider which is still partly implemented on the extension side and run the code that filters/processes document symbol results

vscode-go/src/goTest/resolve.ts

Line 408 in 7df32c8

private async processSymbol(

to support testify and other popular test utilities, etc. This is actually the same code used to place the test code lenses in vscode-go.

As a long term solution, I think we need this test function identification logic to be implemented inside gopls properly.

[gopherbot]

Change https://go.dev/cl/458999 mentions this issue: src/goTest: do not open text document on didCreateFile

[gopherbot]

Change https://go.dev/cl/462376 mentions this issue: src/goTest: do not open text document on didCreateFile

[gopherbot]

Change https://go.dev/cl/462376 mentions this issue: [release] src/goTest: do not open text document on didCreateFile