Integration test code coverage: Add support for merging additional coverage files that were created before the test run completed.

[saborrie]

Is your feature request related to a problem? Please describe.
As well as unit tests, my Go applications will often have a few integration tests that don't call functions from my package, but instead build it into a binary and then execute the binary. I would like these tests to be picked up by vscode-go for reporting and displaying code coverage

Describe the solution you'd like
Go has support in go build to add the -cover option, which will create binaries that report coverage based on the GOCOVERDIR environment variable. Integration tests would create this as an additional coverage file, rather than being added to the one output from the test run itself, which is triggered by vscode-go.

I would like vscode-go to include an additional option such as go.additionalCoverageFiles. I could set this option to a directory such as /tmp/my_go_integration_test_coverage, and then vscode-go can look in that directory after each test run for additonal coverage files, and if found merge them into the main coverage file that was output by the test run.

Describe alternatives you've considered
I have tried to do the merge myself, such as in TestMain, but the main coverage file doesn't exist until after the tests exit, to there is nothing to merge.

Example

// main.go
package main

import (
	"os"
	"fmt"
)

func main() {
	if len(os.Args) < 2 {
		fmt.Println("Please provide a command")
		os.Exit(1) // we can't unit test this behaviour because it exits, an exit causes unit tests to fail
	}
	fmt.Println("Ok")
}

// main_test.go
package main_test

import (
	"bytes"
	"fmt"
	"os"
	"os/exec"
	"testing"
)

var binaryName = "/tmp/my_go_binary_for_integration_testing"
var coverageDir = "/tmp/my_go_integration_test_coverage"

func TestMain(m *testing.M) {
	cmd := exec.Command("go", "build", "-cover",  "-o", binaryName, "./main.go")
	cmd.Stdout = os.Stdout
	cmd.Stderr = os.Stderr
	err := cmd.Run()
	if err != nil {
		fmt.Printf("failed to build binary: %v", err)
		os.Exit(1)
	}

	exitCode := m.Run()

	os.Remove(binaryName)

	os.Exit(exitCode)
}

func TestShouldExitWithCode1WhenNoArgProvided(t *testing.T) {
	var stdout, stderr bytes.Buffer
	cmd := exec.Command(binaryName)
	cmd.Stdout = &stdout
	cmd.Stderr = &stderr
	
	// we can set the coverage directory for the instrumented binary when we execute it
	cmd.Env = []string{"GOCOVERDIR="+coverageDir}

	// Run the command
	err := cmd.Run()

	if err == nil {
		t.Fatalf("expected error from exit code 1, got nil")
	}
}

[firelizzard18]

@saborrie Am I correct that the feature you're requesting is simply:

• After running tests, vscode-go checks for coverage files in go.additionalCoverageFiles?

@hyangah From a technical perspective this shouldn't be hard to add to Go Companion, I just need to scan for files and merge into the coverage map. From a UX perspective, I'm not sure how best to handle this. @saborrie Presumably, you don't want coverage from a previous run to 'pollute' the results of a subsequent run. So those files need to be deleted, or we'd need some way of specifying a new directory for each run. Should it be the extension's responsibility to delete old coverage files at the beginning of a run? From microsoft/vscode#227924 it sounds like there's some possibility that we could get a temp directory for each test run. Should we pass that to the test via an environment variable?

[saborrie]

@firelizzard18, yes that is what I am requesting, but I've dug deeper and I think there might be a simpler solution.

You are right that as I have requested it would suffer from the possibility of those coverage files not getting cleaned out at the right time unless vscode-go took responsibility.

I checked, and running a regular unit test with vscode-go for coverage will have GOCOVERDIR env var set to a temporary directory as well, but it looks to stay the same between runs, so it must be getting cleaned up each time.

Perhaps a better revised setting would be a boolean configuration variable such as go.enableAdditionalCoverageFiles, which would simply enable:

• After running tests, vscode-go checks for additonal coverage files in the same directory it already uses for the main coverage files

Integration tests can then simply check GOCOVERDIR and pass the same value down to the binaries that they run.

What I don't know is whether this can still cause issues from multiple test runs happening simultaneously - I don't know if vscode-go prevents that

[firelizzard18]

I checked, and running a regular unit test with vscode-go for coverage will have GOCOVERDIR env var set to a temporary directory as well, but it looks to stay the same between runs, so it must be getting cleaned up each time.

I'm pretty confident this is not being done by vscode-go, so it must be something go test does. Go Companion generates a test path and then passes -coverprofile, -covermode, and -coverpkg to go test. I just tested, setting GOCOVERDIR when invoking go test does not have an effect. go test must be setting the environment variable. In my test it is a different directory each time, %LocalAppData%\Temp\go-buildXXX\b001\gocoverdir (I'm on Windows ATM) where XXX is different between runs. I could read from that directory but honestly I'm not sure how to determine it (without doing something heinous like modifying the user's tests to print it). I found an issue and documentation on the topic.

After running tests, vscode-go checks for additonal coverage files in the same directory it already uses for the main coverage files

vscode-go does the same thing as Go Companion (I just checked): it uses the -coverprofile flag. I think GOCOVERDIR is an internal mechanism (at least with respect to tests) that go test uses and not something we can mess with. I think the most feasible option is for Go Companion to provide its temp dir via an environment variable like VSCODE_TESTTEMPDIR and leave it up to your test to put coverage files in there one way or another, plus a setting like extraCoveragePattern that specifies where to look for extra coverage in that directory.. But that will be more complicated than I thought, due to Go Companion (and vscode-go) expecting the legacy format mentioned in the documentation I linked.

For context, I am the original author of vscode-go's test explorer implementation, which is distinct from it's testing commands and code lenses, and I am working on replacing both with a new implementation using gopls. That new implementation currently lives in Go Companion. If you install a preview/prerelease version of vscode-go plus Go Companion, vscode-go will switch to using Go Companion for tests. If you're interested in submitting a PR, I can give you pointers.

[saborrie]

From golang/go#66225, maybe Go itself will get an improvement in this area anyway.

It sounds like GOCOVERDIR gets set to a temp directory by go, which would line up with what you've said about -coverprofile being set to a per-run temp directory. Perhaps the one that I saw being reused was this default Go behaviour.

I wonder if part of the issue is that there are multiple formats - it sounds like the instrumented binaries output in a newer format than what happens if you use -coverprofile, so the tests that run these binaries might need to use go tool covdata for the conversion.

[firelizzard18]

I wonder if part of the issue is that there are multiple formats - it sounds like the instrumented binaries output in a newer format than what happens if you use -coverprofile, so the tests that run these binaries might need to use go tool covdata for the conversion.

You're a bit mixed up. The process is (IIUC):

1. With an appropriate flag set, go test creates a binary instrumented for coverage.
2. go test does the equivalent of export GOCOVERDIR=$(mktemp -d) and executes the instrumented binary.
3. A coverage-instrumented binary will output coverage data in the new format to GOCOVERDIR.
4. After the test binary has executed, go test reads coverage data out of GOCOVERDIR;
5. And excludes coverage files originating from binaries that aren't the test binary (runtime/coverage: counter mode clash when testing with instrumented go tools go#57924);
6. Then it outputs that coverage data, converted to the old format, to the file specified by -coverprofile.

-coverprofile implicitly sets -cover. Excluding that, all -coverprofile does is instruct go test to generate a report using the old format. Everything else is a function of -cover, which instruments the binary and instructs to emit coverage data in the new format to GOCOVERDIR.

It sounds like your use case used to work, but golang/go#57924 updated go test to specifically ignore coverage files from other binaries when generating the coverage report. A possible solution is for vscode-go/Go Companion to read coverage files directly out of GOCOVERDIR. Except AFAIK we can't predict what GOCOVERDIR will be and we can't emit it without modifying the user's code (which is not a path I'm willing to take).

[findleyr]

Sorry if I'm misunderstand something here, but wouldn't it also work to have vscode-go manage GOCOVERDIR itself, setting it explicitly before each run and injecting the additional coverage files? @firelizzard18 you said we can't mess with GOCOVERDIR, but from reading https://go.dev/doc/build-cover it looks like we can. Am I missing something?

Regardless, it sounds like all of the proposed solutions here are nontrivial, and so this is unlikely to be prioritized by the team soon. If someone can propose a concrete solution based on the above discussion, we can review and mark this as a candidate for external contribution.

[firelizzard18]

@findleyr I agree that it sounds like that should work but empirically it does not appear to be possible to inject GOCOVERDIR into a go test invocation:

func Test(t *testing.T) {
	t.Log(os.Getenv("GOCOVERDIR"))
}

$ GOCOVERDIR=/tmp/cover go test -cover -v .
=== RUN   Test
    env_test.go:9: /tmp/go-build3091125187/b001/gocoverdir
--- PASS: Test (0.00s)
PASS
coverage: [no statements]
ok      gitlab.com/firelizzard/go-script        0.001s  coverage: [no statements]

I'm guessing this is a feature of go test and not the test binary - the test binary does not have a -test.cover flag. So we could theoretically 'take over' the role go test plays when executing a test binary and handle all the coverage processing ourselves. Among other advantages, that would give us direct access to GOCOVERDIR and the coverage files in the new format. But that's not a trivial task.