Testing API

[firelizzard18]

Does vscode-go intend to implement/participate in the new Testing API for VSCode? I am the author of Go Test Explorer, which integrates with the Test Explorer UI.

If and when this extension implements the Testing API, I will retire my extension.

[hyangah]

Hi @firelizzard18

It looks like the first parts of the proposed testing APIs (microsoft/vscode#107467) are close to finalization and so it is good time to start thinking about integration.

It would be nice if the test exploration (along with coverage, profiling, tracing ..) is integrated into the go extension - so users can use it instead of installing separate extensions. I knew the Go Test Explorer was doing a great job in this space already, and unfortunately we are currently swamped with other high priority tasks. As a result, we never put this in our top priority task and it will take time without help from contributors. Is it possible to reuse part of the Go Test Explorer implementation?

[firelizzard18]

I knew the Go Test Explorer was doing a great job in this space already

Thanks! There are some rough spots I'm not happy about, but I know where all the flaws are shrug.

Is it possible to reuse part of the Go Test Explorer implementation?

I have no objection to vscode-go using my code, and we use the same license. Mainly, the code can be separated into A) locate functions of interest and build a data model, and B) handle execution, etc. Locating functions of interest translates to executing vscode.executeDocumentSymbolProvider on each document and scanning the output for functions that start with Test|Benchmark|Example. The data model is something I invented so I could present a sensible tree to the UI. It works, but it could be better. One of the issues is that workspace updates are not always reflected properly in the data model.

Reusing my implementation comes down to how similar you want it to behave. In a number of aspects the implementation is fairly ugly because my extension doesn't have access to vscode-go internals. So some things could be cut out by reusing the run-test, debug-test, and config infrastructure in vscode-go.

The main implementation of the test explorer hooks is... not pretty. And it will have to be rewritten anyways for the new API. Other than that, the main thing is my data model. It's main purpose is to facilitate running all tests for a package/model/file, and presenting the UI as a tree with module > pkg1 > pkg2 > file > tests. I like that way of doing things, but you may prefer something else. Also, it may be preferable to organize the data in a different structure, and construct that tree structure only when presenting. That may simplify some of the update bugs.

Aside from test and debug, the only additional feature I have amounts to running go test with -cpuprofile=<tmpfile> or -memprofile=<tmpfile> and then presenting the results in a webview.

Once the API is stable, I would be willing to start working on integrating. Is this something you would accept a PR for?

And WRT to the profiling, is that something you want in vscode-go? Given that my current approach boils down to feeding the profile to go tool and dropping the graphviz diagram in a webview. When I said, "I will retire my extension," I was really thinking, "I will rip out everything except the profile bits, so I'll probably retire the old extension and publish under a new name." I have no objections to vscode-go taking over that feature, it's just not something I expect in vscode-go. Of course, a memory/cpu profile explorer could be a far more polished and rich feature than what I have today.

[hyangah]

@firelizzard18 Thanks for the explanation.

It sounds like by accessing the extension internals, tighter and cleaner implementation can be possible, which is a good thing!
For vscode.executeDocumentSymbolProvider, I think we can potentially make gopls do a more precise job by implementing custom commands. (There is a pending issue to replace the Go extension's test/benchmark codelenses computation with gopls's.)

Other than that, the main thing is my data model. It's main purpose is to facilitate running all tests for a package/model/file, and presenting the UI as a tree with module > pkg1 > pkg2 > file > tests. I like that way of doing things, but you may prefer something else.

Your choice of the model seems natural to me too - was there any feature request that proposes different ways organizing tests?

Once the API is stable, I would be willing to start working on integrating. Is this something you would accept a PR for?

Sure! Our review process uses gerrit like other go projects. https://github.com/golang/vscode-go/blob/master/docs/contributing.md#mail-your-change-for-review
You can still use github PRs and the bot will make PRs to gerrit CLs. But I found myself interacting directly in gerrit is a bit easier. Advantages of PRs are that GitHub Review UI allows embedding screenshots and videos though. The choice is up to you.

And WRT to the profiling, is that something you want in vscode-go?

Yes, profiling is part of important tools so it would be nice if vscode-go can offer it as well.
Starting with the graphviz diagram is one option, and I wonder if it's possible to embed pprof's web UI or flamegraph-like view through webview. But we didn't start to think/work on this actively yet. :-P

[firelizzard18]

@hyangah Sounds great! I'll start poking around. No one has objected to the tree structure, so I'll stick with that.

[firelizzard18]

@hyangah go env GOMOD can report the module file as /dev/null under certain circumstances (see also #36052). In this case, getModFolderPath may return /dev as the module file for a package that does not contain a module file. On my system, I see this with $GOPATH/src/github.com/go-gl/glfw.

Is this intended behavior? I assumed getModFolderPath/isModSupported would always return null/false when no go.mod is present.

[hyangah]

@firelizzard18 That sounds like a bug in getModFolderPath.

        GOMOD
                The absolute path to the go.mod of the main module.
                If module-aware mode is enabled, but there is no go.mod, GOMOD will be
                os.DevNull ("/dev/null" on Unix-like systems, "NUL" on Windows).
                If module-aware mode is disabled, GOMOD will be the empty string.

GOMOD = /dev/null means module mode is enabled but there is no go.mod file. With go1.16 that enables module mode by default, I think go build or go test will fail with errors like "go: cannot find main module ... "`.

[firelizzard18]

Test discovery is dynamic - driven by expanding the tree in the explorer - and driven by opening and editing documents.

[firelizzard18]

@hyangah I'm happy to use Gerrit, but it's not clear to me how I'm supposed to use git codereview. From reading the documentation, I thought it would effectively git commit --amend each time I git codereview change, but that doesn't always seem to be the case. Maybe because I changed the tracking branch? As far as I can tell, the first git codereview change set the tracking branch to origin/master, but I definitely don't want to push to master, even though origin is my own fork. So I set it to track a different branch, so I could push my progress to GitHub without clobbering master.

[firelizzard18]

@hyangah I have the basic implementation done.

[firelizzard18]

@hyangah or @suzmue, have you looked at microsoft/vscode#123713? I don't know enough about Go's test coverage to know if the proposed API is sufficient for Go.

[firelizzard18]

For reference (for me): microsoft/vscode#122208

[hyangah]

Thanks @firelizzard18 - Go's coverage instrumentation results are in the form of

 filename:StartLine.StartColumn,EndLine.EndColumn Hits CoverCount

So, I think the proposed API fits to represent one single test (or subtest) run. But maybe @pjweinb who worked on coverage feature improvement recently has a different opinion.

In Go, there is a way to run multiple tests at once (all tests in a package, or in a workspace). AFAIK, the coverage data does not track hit count per individual test. So, I guess, to work with the proposed API, we need to discover all individual tests and run them individually. I think it's up to how the test api & coverage api & vscode UI is wired.

[pjweinb]

hyangah, i think that's right. The coverage results are from a single run.

[firelizzard18]

At the moment, I'm collating a list of function names per package and executing them with -run. Running tests individually wouldn't be hard.

If I'm reading the proposed API correctly, coverage data is per-run, not per-test. If the user triggers a run at the package or module level, that is expected to run all test within that scope, as a single TestRun. So I think my current implementation would be sufficient - once go test returns, I can read the coverage file and attach it to the TestRun.

[firelizzard18]

It sounds like most of the testing API will be finalized/released in the upcoming VSCode release

[hyangah]

Thanks @firelizzard18
I am not sure if there is a way to test this in the Nightly extension - it doesn't seem like there is no way to publish an extension that uses proposed APIs to the marketplace. Am I understanding it correctly?

There are many features that we need a newer vscode engine so we were planning to update our vscode engine requirements. :-)

How about declaring v0.27.1 as the last version that supports vscode 1.52+.
And, make v0.28.0 require 1.59+ (July 2021) and release it after 1.59? Is it too aggressive? @suzmue @stamblerre

[firelizzard18]

It doesn't seem like there is no way to publish an extension that uses proposed APIs to the marketplace. Am I understanding it correctly?

@hyangah Correct, to use a proposed API, you have to set a flag in package.json, and the marketplace will reject an extension with that flag set. You can package the extension and install the vsix file yourself, but I don't think that will work unless you're using the insiders build. Some proposed APIs are available if you launch stable VSCode with a command line flag, but AFAIK others including testing are only available in insiders or source builds.

Update

We only ship stable ~once per month, so the latest set of API changes will not be in stable, you can only test the old API. The next stable release where these changes are included should arrive when this comment is two weeks old.

[firelizzard18]

microsoft/vscode#122208 was just closed, and the API moved from vscode.proposed.d.ts to vscode.d.ts. VSCode Insiders is released on Mondays, so next week I'll update the CL to use the finalized API (and remove vscode.proposed.d.ts.

[connor4312]

Insiders is actually released in the European morning each work day -- so overnight in the US Sunday-Thursday 🙂

[firelizzard18]

Ah, I saw you mention 'not until Monday' to someone, so I assumed it was always Monday.

[firelizzard18]

Due to the lack of log message support for passed tests (microsoft/vscode#129201), benchmarks may change. If I can't find a work around, I may disable them (for now).

[firelizzard18]

It appears that "enableProposedApi": true is still required. Though it doesn't really matter because you have to use Insiders anyways.

[firelizzard18]

OK, my solution for now for benchmarks is:

• By default, running a set of tests (module/package/file) will not run benchmarks
• But you can change that in configuration
• If you run a benchmark directly, it will run
• If you run a set of tests that only includes benchmarks, they will run
• If you select multiple tests (or sets), any selected benchmarks will run

Since there's no longer a way to attach messages to passing test runs, if a benchmark returns results I explicitly call the command that displays the test result terminal.

[firelizzard18]

The PR is updated to the finalized API and has basic support for showing cpu/mem profiles. It still needs polish (and review).

[firelizzard18]

I'm going to move profiling to its own branch and remove it from the CL. The simple implementation just shows a text profile, which isn't super useful, and this CL is already large enough.

[feitian124]

nice work 👍

[gopherbot]

Change https://golang.org/cl/330809 mentions this issue: src/goTestExplorer: implement a test provider for the new test api