testscript: merge coverage from all test binary executions

First, consolidate how top-level commands registered via RunMain are
executed. Before, they could only be run directly, such as "foo <args>".
This worked because the command would run the test binary with a special
TESTSCRIPT_COMMAND env variable, telling the sub-process what command to
run.

The unfortunate side effect was that the commands only worked when
called directly. They wouldn't work when called indirectly, such as
"exec foo" or "go build -toolexec=foo".

To fix that inconsistency, we now set up a directory in $PATH with all
the commands as copies of the test binary. The test binary sub-process
knows what command it should execute thanks to os.Args[0]. This also
lets us get rid of the TESTSCRIPT_COMMAND dance.

Second, make all top-level command executions write coverage profiles if
the -coverprofile test flag was used. Similar to the case before, this
only used to work for direct command executions, not indirect ones. Now
they all get merged into the final coverage profile.

This is accomplished by having them all write coverage profiles under a
shared directory. Once all scripts have run, the parent process walks
that directory and merges all profiles found in it.

Third, add a test that ensures both of the refactors above work well. It
lives under gotooltest, since it runs the real "go test -coverprofile".
It checks all three ways mentioned above to run a top-level command, as
well as checking that all three count towards the total coverage.

Note that some tests needed to be amended to keep working after the
refactor. For example, some tests used a custom "echo" command as well
as the system's "exec echo". Since now both of those would execute the
testscript command, rename that command to fprintargs, which is also
clearer and less ambiguous.

Similarly, dropgofrompath had to be used in one more test, and had to be
adapted to actually do the intended thing on Windows rather than just
emptying the entire PATH variable.

Also swap Go's 1.16beta1 for 1.16rc1 in CI, since the former is now
failing due to a bug in setup-go.

Author: mvdan

.github/workflows/test.yml

@@ -11,7 +11,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go-version: [1.14.x, 1.15.x, 1.16.0-beta1]
+        go-version: [1.14.x, 1.15.x, 1.16.0-rc1]
         os: [ubuntu-latest, macos-latest, windows-latest]
     runs-on: ${{ matrix.os }}
     steps:

cmd/testscript/main_test.go

@@ -70,7 +70,8 @@ func dropgofrompath(ts *testscript.TestScript, neg bool, args []string) {
 	var newPath []string
 	for _, d := range filepath.SplitList(ts.Getenv("PATH")) {
 		getenv := func(k string) string {
-			if k == "PATH" {
+			// Note that Windows and Plan9 use lowercase "path".
+			if strings.ToUpper(k) == "PATH" {
 				return d
 			}
 			return ts.Getenv(k)

cmd/testscript/testdata/nogo.txt

@@ -1,7 +1,10 @@
 # should support skip
 unquote file.txt
 
-env PATH=
+# We can't just set PATH to empty because we need the part of it that
+# contains the command names, so use a special builtin instead.
+dropgofrompath
+
 ! testscript -v file.txt
 stdout 'unknown command "go"'
 stderr 'error running file.txt in'

gotooltest/script_test.go

@@ -5,6 +5,8 @@
 package gotooltest_test
 
 import (
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/rogpeppe/go-internal/gotooltest"
@@ -14,7 +16,19 @@ import (
 func TestSimple(t *testing.T) {
 	p := testscript.Params{
 		Dir: "testdata",
+		Setup: func(env *testscript.Env) error {
+			// cover.txt will need testscript as a dependency.
+			// Tell it where our module is, via an absolute path.
+			wd, err := os.Getwd()
+			if err != nil {
+				return err
+			}
+			modPath := filepath.Dir(wd)
+			env.Setenv("GOINTERNAL_MODULE", modPath)
+			return nil
+		},
 	}
+
 	if err := gotooltest.Setup(&p); err != nil {
 		t.Fatal(err)
 	}

gotooltest/testdata/cover.txt

@@ -0,0 +1,82 @@
+unquote scripts/exec.txt
+
+# The module uses testscript itself.
+# Use the checked out module, based on where the test binary ran.
+go mod edit -replace=github.com/rogpeppe/go-internal=${GOINTERNAL_MODULE}
+go mod tidy
+
+# First, a 'go test' run without coverage.
+go test -vet=off
+stdout 'PASS'
+! stdout 'total coverage'
+
+# Then, a 'go test' run with -coverprofile.
+# Assuming testscript works well, this results in the basic coverage being 0%,
+# since the test binary does not directly run any non-test code.
+# The total coverage after merging profiles should end up being 100%,
+# as long as all three sub-profiles are accounted for.
+# Marking all printlns as covered requires all edge cases to work well.
+go test -vet=off -coverprofile=cover.out -v
+stdout 'PASS'
+stdout '^coverage: 0\.0%'
+stdout '^total coverage: 100\.0%'
+! stdout 'malformed coverage' # written by "go test" if cover.out is invalid
+exists cover.out
+
+-- go.mod --
+module test
+
+go 1.15
+-- foo.go --
+package foo
+
+import "os"
+
+func foo1() int {
+	switch os.Args[1] {
+	case "1":
+		println("first path")
+	case "2":
+		println("second path")
+	default:
+		println("third path")
+	}
+	return 1
+}
+-- foo_test.go --
+package foo
+
+import (
+	"os"
+	"testing"
+
+	"github.com/rogpeppe/go-internal/gotooltest"
+	"github.com/rogpeppe/go-internal/testscript"
+)
+
+func TestMain(m *testing.M) {
+	os.Exit(testscript.RunMain(m, map[string] func() int{
+		"foo": foo1,
+	}))
+}
+
+func TestFoo(t *testing.T) {
+	p := testscript.Params{
+		Dir: "scripts",
+	}
+	if err := gotooltest.Setup(&p); err != nil {
+		t.Fatal(err)
+	}
+	testscript.Run(t, p)
+}
+-- scripts/exec.txt --
+># Note that foo always fails, to prevent "go build" from doing anything.
+>
+># Running the command directly; trigger the first path.
+>! foo 1
+>
+># Running the command via exec; trigger the second path.
+>! exec foo 2
+>
+># Running the command indirectly, via toolexec; trigger the third path.
+>! go build -a -toolexec=foo runtime

testscript/cover.go

@@ -8,8 +8,8 @@ import (
 	"bufio"
 	"fmt"
 	"io"
-	"log"
 	"os"
+	"path/filepath"
 	"regexp"
 	"strconv"
 	"strings"
@@ -22,8 +22,13 @@ import (
 // mergeCoverProfile merges the coverage information in f into
 // cover. It assumes that the coverage information in f is
 // always produced from the same binary for every call.
-func mergeCoverProfile(cover *testing.Cover, r io.Reader) error {
-	scanner, err := newProfileScanner(r)
+func mergeCoverProfile(cover *testing.Cover, path string) error {
+	f, err := os.Open(path)
+	if err != nil {
+		return err
+	}
+	defer f.Close()
+	scanner, err := newProfileScanner(f)
 	if err != nil {
 		return errors.Wrap(err)
 	}
@@ -83,45 +88,45 @@ func mergeCoverProfile(cover *testing.Cover, r io.Reader) error {
 	return nil
 }
 
-var (
-	coverChan chan *os.File
-	coverDone chan testing.Cover
-)
-
-func goCoverProfileMerge() {
-	if coverChan != nil {
-		panic("RunMain called twice!")
-	}
-	coverChan = make(chan *os.File)
-	coverDone = make(chan testing.Cover)
-	go mergeCoverProfiles()
-}
-
-func mergeCoverProfiles() {
+func finalizeCoverProfile(dir string) error {
+	// Merge all the coverage profiles written by test binary sub-processes,
+	// such as those generated by executions of commands.
 	var cover testing.Cover
-	for f := range coverChan {
-		if err := mergeCoverProfile(&cover, f); err != nil {
-			log.Printf("cannot merge coverage profile from %v: %v", f.Name(), err)
+	if err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
+		}
+		if !info.Mode().IsRegular() {
+			return nil
+		}
+		if err := mergeCoverProfile(&cover, path); err != nil {
+			return fmt.Errorf("cannot merge coverage profile from %v: %v", path, err)
 		}
-		f.Close()
-		os.Remove(f.Name())
+		return nil
+	}); err != nil {
+		return errors.Wrap(err)
+	}
+	if err := os.RemoveAll(dir); err != nil {
+		// The RemoveAll seems to fail very rarely, with messages like
+		// "directory not empty". It's unclear why.
+		// For now, if it happens again, try to print a bit more info.
+		filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
+			if err == nil && !info.IsDir() {
+				fmt.Fprintln(os.Stderr, "non-directory found after RemoveAll:", path)
+			}
+			return nil
+		})
+		return errors.Wrap(err)
 	}
-	coverDone <- cover
-}
 
-func finalizeCoverProfile() error {
+	// We need to include our own top-level coverage profile too.
 	cprof := coverProfile()
-	if cprof == "" {
-		return nil
+	if err := mergeCoverProfile(&cover, cprof); err != nil {
+		return fmt.Errorf("cannot merge coverage profile from %v: %v", cprof, err)
 	}
-	f, err := os.Open(cprof)
-	if err != nil {
-		return errors.Notef(err, nil, "cannot open existing cover profile")
-	}
-	coverChan <- f
-	close(coverChan)
-	cover := <-coverDone
-	f, err = os.Create(cprof)
+
+	// Finally, write the resulting merged profile.
+	f, err := os.Create(cprof)
 	if err != nil {
 		return errors.Notef(err, nil, "cannot create cover profile")
 	}