x/tools/gopls: integrate test discovery

[firelizzard18]

I would like to migrate the test discovery services of vscode-go into gopls so they can be implemented in Go and utilize the rich type information provided by the go/* packages instead of the current JavaScript implementation which relies on recursive directory walks and analyzing document symbols. Moving test discovery into gopls should improve performance and would bake it far easier to implement improvements such as detecting subtests in cases where that can be done deterministically.

CC @hyangah @suzmue

Related: golang/vscode-go#2445, golang/vscode-go#1602

[firelizzard18]

@gopherbot, please add label FeatureRequest

[findleyr]

See also golang/vscode-go#2719.

We should do this. The actual analysis is not that hard: there are examples of recognizing test functions in the tests analyzer and subtests in the loopclosure analyzer.

We already have a run test code lens that could be intercepted in the vs code middleware. @hyangah @firelizzard18 would it be sufficient to extend that code lens to subtests?

[firelizzard18]

@findleyr from an information perspective that would be sufficient if the code lens messages include some additional information (listed below). Otherwise we'd still need document symbols or some other mechanism for extracting that. However it seems like that might be problematic from a usability perspective. A user may wish to use the test explorer exclusively and disable the test code lens - if the user disables test code lenses does that tell gopls to stop sending those messages?

Off the top of my head I think these are the components we need:

1. List packages within the workspace/module that contain tests/test files.
   • This seems like something best implemented as a separate call to gopls.
2. List files within a package that contain tests/are test files.
   • It's not hard to do this in VSCode/JS via filesystem calls but presumably gopls already has that information loaded into memory so it would be faster to ask gopls.
   • Could this be merged with (1)?
3. List tests within a file.
   • This requires the test name, the range in the file (of the TestXxx function or of the subtest callback), and whether it's a child of another test. Can we reasonably pack all of that into CodeLens.data?
   • This would involve interrogating files that are not open. How difficult is it to request code lenses for a file that has not been opened? What is the performance impact of doing so?

With respect to (1) and (2), testing for a _test.go suffix is sufficient (that's the current behavior) but it would be preferable to verify that the file actually contains tests. I sometimes have util_test.go files that contain shared test harnessing/utilities/etc and do not contain any tests. With the current behavior, those appear in the list and then disappear once they are expanded and discovered to contain no tests. If gopls can quickly and efficiently determine if a file contains tests, using gopls to implement (1) and (2) would improve the user experience.

A big motivation for moving (1) into gopls is to address performance issues on large projects (see golang/vscode-go#2504). The current implementation recursively walks the filesystem, which does not perform well on large projects (and likely performs badly if the project is on a remote filesystem). I realize that gopls will also have to walk the filesystem, but it must already be doing that (to supply type information) and I theorize that using the build metadata gopls collects will perform much better than the current JS implementation.

[firelizzard18]

I am more than willing to work on this myself but I don't know when I'll have the time.

[findleyr]

@firelizzard18 thanks for the details.

It sounds like it may be cleanest to have a custom command, as code lenses in general require type information and we may be able to implement this query without fully type-checking. This command may also have a life-cycle that is different from code lenses, typically.

If we do choose to implement this with type information, we can extract test information as part of our package post-processing, and serialize them in our file cache, so that they will be quickly available at startup. This aspect of the change would be tricker for an external contributor.

I have slated this for gopls@v0.13.0, as this keeps cropping up.

[firelizzard18]

@findleyr I think we can separate the major areas of improvement into three groups:

1. Package discovery
   • The current implementation (walking the filesystem) is ok but it's not very clean and it performs badly for large projects.
2. Test discovery
   • The current implementation (using document symbols) is ok but it's not very clean.
   • I am pretty sure this can be done without type checking, all it really needs to do is locate func Test{name}(*testing.T) declarations.
3. Static subtest discovery.
   • This is not currently supported except in an approximate way for stretchr test suites.
   • It seems to me this would be much easier to implement using complete type information. There are some cases where it's straight-forward to identify subtests by walking the AST, but type information would be very useful for more complex cases.

I think (1) and (2) are orthogonal and could be implemented separately, if that is more convenient. Whether (3) is dependent on (2) depends on the implementation. (3) could be implemented as an improvement to the normal test discovery; alternatively it could be implemented as a second pass.

Static subtest discovery (3) is the part I find most technically interesting. To be clear I know it is impossible to statically discover all sub tests (the halting problem) so IMO the target should be "statically discover subtests in cases where it is feasible to do so" as a companion to dynamic subtest discovery (that is, analyzing test output). I have functionally zero knowledge of how gopls works internally, so maybe I should wait until the team has implemented some form of static subtest discovery and then I can provide contributions building on that.

[mnoah1]

Package discovery

• The current implementation (walking the filesystem) is ok but it's not very clean and it performs badly for large projects.

Can this improvement be done in away that it considers (or at least leaves the option open for) an option to infer relevant packages from the user's current open workspace state? As we were discussing on golang/vscode-go#2504, to support a large monorepo environment it becomes not just an issue of indexing speed, but also of relevance.

When many teams use one monorepo, "Show everything" (the current behavior) becomes too broad and shows thousands of irrelevant packages, and switching to nested mode becomes very deep. On the other hand, "Show only the package of the file I have open" may be too narrow, but is consistent, intuitive, and doesn't require the user to maintain any separate config with relevant paths. Perhaps the solution could be something like limiting the display to packages within "x" levels of the current directory.