From 85c8689ce5139e3ec22609b2147ec3fb5556f193 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Wed, 27 Jan 2021 16:52:40 +0000 Subject: [PATCH] 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 ". 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. --- .github/workflows/test.yml | 2 +- cmd/testscript/main_test.go | 3 +- cmd/testscript/testdata/nogo.txt | 5 +- gotooltest/script_test.go | 14 ++ gotooltest/testdata/cover.txt | 82 +++++++++ testscript/cover.go | 68 ++++---- testscript/exe.go | 164 +++++++++++++----- testscript/testdata/readfile.txt | 4 +- .../testdata/testscript_update_script.txt | 4 +- ..._update_script_expected_not_in_archive.txt | 2 +- .../testscript_update_script_quote.txt | 4 +- .../testscript_update_script_stderr.txt | 4 +- testscript/testscript.go | 4 + testscript/testscript_test.go | 4 +- 14 files changed, 267 insertions(+), 97 deletions(-) create mode 100644 gotooltest/testdata/cover.txt diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ec593d61..311ed7d3 100644 --- a/.github/workflows/test.yml +++ b/.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: diff --git a/cmd/testscript/main_test.go b/cmd/testscript/main_test.go index ac7f63fb..7bc39028 100644 --- a/cmd/testscript/main_test.go +++ b/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) diff --git a/cmd/testscript/testdata/nogo.txt b/cmd/testscript/testdata/nogo.txt index e3dd684d..601872b2 100644 --- a/cmd/testscript/testdata/nogo.txt +++ b/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' diff --git a/gotooltest/script_test.go b/gotooltest/script_test.go index 47e6fe72..5beb54a7 100644 --- a/gotooltest/script_test.go +++ b/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) } diff --git a/gotooltest/testdata/cover.txt b/gotooltest/testdata/cover.txt new file mode 100644 index 00000000..dee60dd7 --- /dev/null +++ b/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 diff --git a/testscript/cover.go b/testscript/cover.go index 09400c95..560d72bb 100644 --- a/testscript/cover.go +++ b/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,36 @@ 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 { + 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 - } - f, err := os.Open(cprof) - if err != nil { - return errors.Notef(err, nil, "cannot open existing cover profile") + if err := mergeCoverProfile(&cover, cprof); err != nil { + return fmt.Errorf("cannot merge coverage profile from %v: %v", cprof, err) } - 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") } diff --git a/testscript/exe.go b/testscript/exe.go index e869e55d..8f840285 100644 --- a/testscript/exe.go +++ b/testscript/exe.go @@ -5,17 +5,19 @@ package testscript import ( + cryptorand "crypto/rand" "flag" "fmt" "io" + "io/ioutil" "log" "os" - "sync/atomic" + "path/filepath" + "runtime" + "strings" "testing" ) -var profileId int32 = 0 - // TestingM is implemented by *testing.M. It's defined as an interface // to allow testscript to co-exist with other testing frameworks // that might also wish to call M.Run. @@ -49,63 +51,138 @@ func IgnoreMissedCoverage() { // // This function returns an exit code to pass to os.Exit, after calling m.Run. func RunMain(m TestingM, commands map[string]func() int) (exitCode int) { - goCoverProfileMerge() - cmdName := os.Getenv("TESTSCRIPT_COMMAND") - if cmdName == "" { + // Depending on os.Args[0], this is either the top-level execution of + // the test binary by "go test", or the execution of one of the provided + // commands via "foo" or "exec foo". + + cmdName := filepath.Base(os.Args[0]) + if runtime.GOOS == "windows" { + cmdName = strings.TrimSuffix(cmdName, ".exe") + } + mainf := commands[cmdName] + if mainf == nil { + // Unknown command; this is just the top-level execution of the + // test binary by "go test". + + // Set up all commands in a directory, added in $PATH. + tmpdir, err := ioutil.TempDir("", "testscript-main") + if err != nil { + log.Printf("could not set up temporary directory: %v", err) + return 2 + } defer func() { - if err := finalizeCoverProfile(); err != nil { - log.Printf("cannot merge cover profiles: %v", err) + if err := os.RemoveAll(tmpdir); err != nil { + log.Printf("cannot delete temporary directory: %v", err) exitCode = 2 } }() + bindir := filepath.Join(tmpdir, "bin") + if err := os.MkdirAll(bindir, 0777); err != nil { + log.Printf("could not set up PATH binary directory: %v", err) + return 2 + } + os.Setenv("PATH", bindir+string(filepath.ListSeparator)+os.Getenv("PATH")) + + flag.Parse() + // If we are collecting a coverage profile, set up a shared + // directory for all executed test binary sub-processes to write + // their profiles to. Before finishing, we'll merge all of those + // profiles into the main profile. + if coverProfile() != "" { + coverdir := filepath.Join(tmpdir, "cover") + if err := os.MkdirAll(coverdir, 0777); err != nil { + log.Printf("could not set up cover directory: %v", err) + return 2 + } + os.Setenv("TESTSCRIPT_COVER_DIR", coverdir) + defer func() { + if err := finalizeCoverProfile(coverdir); err != nil { + log.Printf("cannot merge cover profiles: %v", err) + exitCode = 2 + } + }() + } + // We're not in a subcommand. for name := range commands { name := name + // Set up this command in the directory we added to $PATH. + binfile := filepath.Join(bindir, name) + if runtime.GOOS == "windows" { + binfile += ".exe" + } + binpath, err := os.Executable() + if err == nil { + err = copyBinary(binpath, binfile) + } + if err != nil { + log.Printf("could not set up %s in $PATH: %v", name, err) + return 2 + } scriptCmds[name] = func(ts *TestScript, neg bool, args []string) { - path, err := os.Executable() - if err != nil { - ts.Fatalf("cannot determine path to test binary: %v", err) - } - id := atomic.AddInt32(&profileId, 1) - 1 - oldEnvLen := len(ts.env) - cprof := coverFilename(id) - ts.env = append(ts.env, - "TESTSCRIPT_COMMAND="+name, - "TESTSCRIPT_COVERPROFILE="+cprof, - ) - ts.cmdExec(neg, append([]string{path}, args...)) - ts.env = ts.env[0:oldEnvLen] - if cprof == "" { - return - } - f, err := os.Open(cprof) - if err != nil { - if ignoreMissedCoverage { - return - } - ts.Fatalf("command %s (args %q) failed to generate coverage information", name, args) - return - } - coverChan <- f + ts.cmdExec(neg, append([]string{name}, args...)) } } return m.Run() } - mainf := commands[cmdName] - if mainf == nil { - log.Printf("unknown command name %q", cmdName) - return 2 - } // The command being registered is being invoked, so run it, then exit. os.Args[0] = cmdName - cprof := os.Getenv("TESTSCRIPT_COVERPROFILE") - if cprof == "" { + coverdir := os.Getenv("TESTSCRIPT_COVER_DIR") + if coverdir == "" { // No coverage, act as normal. return mainf() } + + // For a command "foo", write ${TESTSCRIPT_COVER_DIR}/foo-${RANDOM}. + // Note that we do not use ioutil.TempFile as that creates the file. + // In this case, we want to leave it to -test.coverprofile to create the + // file, as otherwise we could end up with an empty file. + // Later, when merging profiles, trying to merge an empty file would + // result in a confusing error. + rnd, err := nextRandom() + if err != nil { + log.Printf("could not obtain random number: %v", err) + return 2 + } + cprof := filepath.Join(coverdir, fmt.Sprintf("%s-%x", cmdName, rnd)) return runCoverSubcommand(cprof, mainf) } +func nextRandom() ([]byte, error) { + p := make([]byte, 6) + _, err := cryptorand.Read(p) + return p, err +} + +// copyBinary makes a copy of a binary to a new location. It is used as part of +// setting up top-level commands in $PATH. +// +// It does not attempt to use symlinks for two reasons: +// +// First, some tools like cmd/go's -toolexec will be clever enough to realise +// when they're given a symlink, and they will use the symlink target for +// executing the program. This breaks testscript, as we depend on os.Args[0] to +// know what command to run. +// +// Second, symlinks might not be available on some environments, so we have to +// implement a "full copy" fallback anyway. +func copyBinary(from, to string) error { + writer, err := os.OpenFile(to, os.O_WRONLY|os.O_CREATE, 0777) + if err != nil { + return err + } + defer writer.Close() + + reader, err := os.Open(from) + if err != nil { + return err + } + defer reader.Close() + + _, err = io.Copy(writer, reader) + return err +} + // runCoverSubcommand runs the given function, then writes any generated // coverage information to the cprof file. // This is called inside a separately run executable. @@ -158,13 +235,6 @@ func runCoverSubcommand(cprof string, mainf func() int) (exitCode int) { return mainf() } -func coverFilename(id int32) string { - if cprof := coverProfile(); cprof != "" { - return fmt.Sprintf("%s_%d", cprof, id) - } - return "" -} - func coverProfileFlag() flag.Getter { f := flag.CommandLine.Lookup("test.coverprofile") if f == nil { diff --git a/testscript/testdata/readfile.txt b/testscript/testdata/readfile.txt index 110986b7..dd08b628 100644 --- a/testscript/testdata/readfile.txt +++ b/testscript/testdata/readfile.txt @@ -1,7 +1,7 @@ -echo stdout stdout +fprintargs stdout stdout testreadfile stdout -echo stderr stderr +fprintargs stderr stderr testreadfile stderr testreadfile x/somefile diff --git a/testscript/testdata/testscript_update_script.txt b/testscript/testdata/testscript_update_script.txt index 43a39883..db481ec8 100644 --- a/testscript/testdata/testscript_update_script.txt +++ b/testscript/testdata/testscript_update_script.txt @@ -4,13 +4,13 @@ testscript-update scripts cmp scripts/testscript.txt testscript-new.txt -- scripts/testscript.txt -- ->echo stdout right +>fprintargs stdout right >cmp stdout expect > >-- expect -- >wrong -- testscript-new.txt -- ->echo stdout right +>fprintargs stdout right >cmp stdout expect > >-- expect -- diff --git a/testscript/testdata/testscript_update_script_expected_not_in_archive.txt b/testscript/testdata/testscript_update_script_expected_not_in_archive.txt index 81766a65..cda575c9 100644 --- a/testscript/testdata/testscript_update_script_expected_not_in_archive.txt +++ b/testscript/testdata/testscript_update_script_expected_not_in_archive.txt @@ -6,7 +6,7 @@ cp scripts/testscript.txt unchanged cmp scripts/testscript.txt unchanged -- scripts/testscript.txt -- ->echo stdout right +>fprintargs stdout right >cp file expect >cmp stdout expect > diff --git a/testscript/testdata/testscript_update_script_quote.txt b/testscript/testdata/testscript_update_script_quote.txt index ae6b00ba..a634b8b8 100644 --- a/testscript/testdata/testscript_update_script_quote.txt +++ b/testscript/testdata/testscript_update_script_quote.txt @@ -4,13 +4,13 @@ testscript-update scripts cmp scripts/testscript.txt testscript-new.txt -- scripts/testscript.txt -- ->echo stdout '-- lookalike --' +>fprintargs stdout '-- lookalike --' >cmp stdout expect > >-- expect -- >wrong -- testscript-new.txt -- ->echo stdout '-- lookalike --' +>fprintargs stdout '-- lookalike --' >cmp stdout expect > >-- expect -- diff --git a/testscript/testdata/testscript_update_script_stderr.txt b/testscript/testdata/testscript_update_script_stderr.txt index e321631f..66882b83 100644 --- a/testscript/testdata/testscript_update_script_stderr.txt +++ b/testscript/testdata/testscript_update_script_stderr.txt @@ -4,13 +4,13 @@ testscript-update scripts cmp scripts/testscript.txt testscript-new.txt -- scripts/testscript.txt -- ->echo stderr right +>fprintargs stderr right >cmp stderr expect > >-- expect -- >wrong -- testscript-new.txt -- ->echo stderr right +>fprintargs stderr right >cmp stderr expect > >-- expect -- diff --git a/testscript/testscript.go b/testscript/testscript.go index d915fda1..495a6e0a 100644 --- a/testscript/testscript.go +++ b/testscript/testscript.go @@ -299,6 +299,10 @@ func (ts *TestScript) setup() string { "devnull=" + os.DevNull, "/=" + string(os.PathSeparator), ":=" + string(os.PathListSeparator), + + // If we are collecting coverage profiles for merging into the main one, + // ensure the environment variable is forwarded to sub-processes. + "TESTSCRIPT_COVER_DIR=" + os.Getenv("TESTSCRIPT_COVER_DIR"), }, WorkDir: ts.workdir, Values: make(map[interface{}]interface{}), diff --git a/testscript/testscript_test.go b/testscript/testscript_test.go index a28e3098..0fef259c 100644 --- a/testscript/testscript_test.go +++ b/testscript/testscript_test.go @@ -26,7 +26,7 @@ func printArgs() int { return 0 } -func echo() int { +func fprintArgs() int { s := strings.Join(os.Args[2:], " ") switch os.Args[1] { case "stdout": @@ -60,7 +60,7 @@ func signalCatcher() int { func TestMain(m *testing.M) { os.Exit(RunMain(m, map[string]func() int{ "printargs": printArgs, - "echo": echo, + "fprintargs": fprintArgs, "status": exitWithStatus, "signalcatcher": signalCatcher, }))