From 26086ede6493449c3885dba0785bdc1b45b2baa1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Wed, 8 Feb 2023 14:34:56 +0000 Subject: [PATCH] testscript: remove our code coverage mechanism thanks to Go 1.20 Our code was a fairly hacky version of what Go 1.20 does for us, since we had to externally reach into the testing internals to do the right thing before and after each program execution. With Go 1.20, all we actually need to do is ensure that the GOCOVERDIR environment variable is forwarded properly. With that, test binaries will know how to produce multiple coverage profiles, and "go test" will know how to collect and merge them. We could keep our hacky workaround for the sake of "deep" coverage information for Go 1.19, but that doesn't seem worthwhile. The old mechanism caused test flakes like #130, is incompatible with Go 1.20, and is overall worse than what Go 1.20 can do. If a user wants code coverage with the latest testscript version, they can use Go 1.20. If they are stuck on Go 1.19 and need code coverage, I imagine they can also stick to a slightly older testscript version. On a main package, the old testscript with Go 1.19 reports: PASS coverage: 8.0% of statements total coverage: 90.1% of statements ok mvdan.cc/sh/v3/cmd/shfmt 0.063s The new testscript with Go 1.20 reports: PASS mvdan.cc/sh/v3/cmd/shfmt coverage: 90.1% of statements ok mvdan.cc/sh/v3/cmd/shfmt 0.047s Fixes #130, as the RemoveAll call is no longer present. Fixes #161, as the API is now deprecated. Fixes #199, as "go test -coverprofile" now works on Go 1.20. --- gotooltest/testdata/cover.txt | 15 +- testscript/cover.go | 280 ---------------------------------- testscript/exe.go | 122 +-------------- testscript/testscript.go | 2 +- 4 files changed, 9 insertions(+), 410 deletions(-) delete mode 100644 testscript/cover.go diff --git a/gotooltest/testdata/cover.txt b/gotooltest/testdata/cover.txt index b3d13e26..18a3b169 100644 --- a/gotooltest/testdata/cover.txt +++ b/gotooltest/testdata/cover.txt @@ -1,7 +1,5 @@ unquote scripts/exec.txt -[darwin] skip 'Pending a fix for github.com/rogpeppe/go-internal/issues/130' - # 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} @@ -10,18 +8,17 @@ go mod tidy # First, a 'go test' run without coverage. go test -vet=off stdout 'PASS' -! stdout 'total coverage' +! stdout '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. +# The total coverage after merging profiles should end up being 100%. # Marking all printlns as covered requires all edge cases to work well. +# Go 1.20 learned to produce and merge multiple coverage profiles, +# so versions before then report a shallow 0% coverage. go test -vet=off -coverprofile=cover.out -v stdout 'PASS' -stdout '^coverage: 0\.0%' -stdout '^total coverage: 100\.0%' +[go1.20] stdout 'coverage: 100\.0%' +[!go1.20] stdout 'coverage: 0\.0%' ! stdout 'malformed coverage' # written by "go test" if cover.out is invalid exists cover.out diff --git a/testscript/cover.go b/testscript/cover.go deleted file mode 100644 index 181605b0..00000000 --- a/testscript/cover.go +++ /dev/null @@ -1,280 +0,0 @@ -// Copyright 2018 The Go Authors. All rights reserved. -// Use of this source code is governed by a BSD-style -// license that can be found in the LICENSE file. - -package testscript - -import ( - "bufio" - "errors" - "fmt" - "io" - "os" - "path/filepath" - "regexp" - "strconv" - "strings" - "sync/atomic" - "testing" -) - -// 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, path string) error { - f, err := os.Open(path) - if err != nil { - return err - } - defer f.Close() - scanner, err := newProfileScanner(f) - if err != nil { - return err - } - if scanner.Mode() != testing.CoverMode() { - return errors.New("unexpected coverage mode in subcommand") - } - if cover.Mode == "" { - cover.Mode = scanner.Mode() - } - isCount := cover.Mode == "count" - if cover.Counters == nil { - cover.Counters = make(map[string][]uint32) - cover.Blocks = make(map[string][]testing.CoverBlock) - } - - // Note that we rely on the fact that the coverage is written - // out file-by-file, with all blocks for a file in sequence. - var ( - filename string - blockId uint32 - counters []uint32 - blocks []testing.CoverBlock - ) - flush := func() { - if len(counters) > 0 { - cover.Counters[filename] = counters - cover.Blocks[filename] = blocks - } - } - for scanner.Scan() { - block := scanner.Block() - if scanner.Filename() != filename { - flush() - filename = scanner.Filename() - counters = cover.Counters[filename] - blocks = cover.Blocks[filename] - blockId = 0 - } else { - blockId++ - } - if int(blockId) >= len(counters) { - counters = append(counters, block.Count) - blocks = append(blocks, block.CoverBlock) - continue - } - // TODO check that block.CoverBlock == blocks[blockId] ? - if isCount { - counters[blockId] += block.Count - } else { - counters[blockId] |= block.Count - } - } - flush() - if scanner.Err() != nil { - return fmt.Errorf("error scanning profile: %v", err) - } - return nil -} - -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 - 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) - } - return nil - }); err != nil { - return 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 err - } - - // We need to include our own top-level coverage profile too. - cprof := coverProfile() - if err := mergeCoverProfile(&cover, cprof); err != nil { - return fmt.Errorf("cannot merge coverage profile from %v: %v", cprof, err) - } - - // Finally, write the resulting merged profile. - f, err := os.Create(cprof) - if err != nil { - return fmt.Errorf("cannot create cover profile: %v", err) - } - defer f.Close() - w := bufio.NewWriter(f) - if err := writeCoverProfile1(w, cover); err != nil { - return err - } - if err := w.Flush(); err != nil { - return err - } - if err := f.Close(); err != nil { - return err - } - return nil -} - -func writeCoverProfile1(w io.Writer, cover testing.Cover) error { - fmt.Fprintf(w, "mode: %s\n", cover.Mode) - var active, total int64 - var count uint32 - for name, counts := range cover.Counters { - blocks := cover.Blocks[name] - for i := range counts { - stmts := int64(blocks[i].Stmts) - total += stmts - count = atomic.LoadUint32(&counts[i]) // For -mode=atomic. - if count > 0 { - active += stmts - } - _, err := fmt.Fprintf(w, "%s:%d.%d,%d.%d %d %d\n", name, - blocks[i].Line0, blocks[i].Col0, - blocks[i].Line1, blocks[i].Col1, - stmts, - count, - ) - if err != nil { - return err - } - } - } - if total == 0 { - total = 1 - } - fmt.Printf("total coverage: %.1f%% of statements%s\n", 100*float64(active)/float64(total), cover.CoveredPackages) - return nil -} - -type profileScanner struct { - mode string - err error - scanner *bufio.Scanner - filename string - block coverBlock -} - -type coverBlock struct { - testing.CoverBlock - Count uint32 -} - -var profileLineRe = regexp.MustCompile(`^(.+):([0-9]+)\.([0-9]+),([0-9]+)\.([0-9]+) ([0-9]+) ([0-9]+)$`) - -func toInt(s string) int { - i, err := strconv.Atoi(s) - if err != nil { - panic(err) - } - return i -} - -func newProfileScanner(r io.Reader) (*profileScanner, error) { - s := &profileScanner{ - scanner: bufio.NewScanner(r), - } - // First line is "mode: foo", where foo is "set", "count", or "atomic". - // Rest of file is in the format - // encoding/base64/base64.go:34.44,37.40 3 1 - // where the fields are: name.go:line.column,line.column numberOfStatements count - if !s.scanner.Scan() { - return nil, fmt.Errorf("no lines found in profile: %v", s.Err()) - } - line := s.scanner.Text() - mode := strings.TrimPrefix(line, "mode: ") - if len(mode) == len(line) { - return nil, fmt.Errorf("bad mode line %q", line) - } - s.mode = mode - return s, nil -} - -// Mode returns the profile's coverage mode (one of "atomic", "count: -// or "set"). -func (s *profileScanner) Mode() string { - return s.mode -} - -// Err returns any error encountered when scanning a profile. -func (s *profileScanner) Err() error { - if s.err == io.EOF { - return nil - } - return s.err -} - -// Block returns the most recently scanned profile block, or the zero -// block if Scan has not been called or has returned false. -func (s *profileScanner) Block() coverBlock { - if s.err == nil { - return s.block - } - return coverBlock{} -} - -// Filename returns the filename of the most recently scanned profile -// block, or the empty string if Scan has not been called or has -// returned false. -func (s *profileScanner) Filename() string { - if s.err == nil { - return s.filename - } - return "" -} - -// Scan scans the next line in a coverage profile and reports whether -// a line was found. -func (s *profileScanner) Scan() bool { - if s.err != nil { - return false - } - if !s.scanner.Scan() { - s.err = io.EOF - return false - } - m := profileLineRe.FindStringSubmatch(s.scanner.Text()) - if m == nil { - s.err = fmt.Errorf("line %q doesn't match expected format %v", m, profileLineRe) - return false - } - s.filename = m[1] - s.block = coverBlock{ - CoverBlock: testing.CoverBlock{ - Line0: uint32(toInt(m[2])), - Col0: uint16(toInt(m[3])), - Line1: uint32(toInt(m[4])), - Col1: uint16(toInt(m[5])), - Stmts: uint16(toInt(m[6])), - }, - Count: uint32(toInt(m[7])), - } - return true -} diff --git a/testscript/exe.go b/testscript/exe.go index 46c611ae..11e4cfa4 100644 --- a/testscript/exe.go +++ b/testscript/exe.go @@ -5,9 +5,7 @@ package testscript import ( - cryptorand "crypto/rand" "flag" - "fmt" "io" "io/ioutil" "log" @@ -24,14 +22,12 @@ type TestingM interface { Run() int } -var ignoreMissedCoverage = false - // IgnoreMissedCoverage causes any missed coverage information // (for example when a function passed to RunMain // calls os.Exit, for example) to be ignored. // This function should be called before calling RunMain. func IgnoreMissedCoverage() { - ignoreMissedCoverage = true + // XXX: now a no-op } // RunMain should be called within a TestMain function to allow @@ -85,24 +81,6 @@ func RunMain(m TestingM, commands map[string]func() int) (exitCode int) { 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, 0o777); 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 { @@ -131,31 +109,7 @@ func RunMain(m TestingM, commands map[string]func() int) (exitCode int) { } // The command being registered is being invoked, so run it, then exit. os.Args[0] = cmdName - 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 + return mainf() } // copyBinary makes a copy of a binary to a new location. It is used as part of @@ -197,78 +151,6 @@ func copyBinary(from, to string) error { 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. -func runCoverSubcommand(cprof string, mainf func() int) (exitCode int) { - // Change the error handling mode to PanicOnError - // so that in the common case of calling flag.Parse in main we'll - // be able to catch the panic instead of just exiting. - flag.CommandLine.Init(flag.CommandLine.Name(), flag.PanicOnError) - defer func() { - panicErr := recover() - if err, ok := panicErr.(error); ok { - // The flag package will already have printed this error, assuming, - // that is, that the error was created in the flag package. - // TODO check the stack to be sure it was actually raised by the flag package. - exitCode = 2 - if err == flag.ErrHelp { - exitCode = 0 - } - panicErr = nil - } - // Set os.Args so that flag.Parse will tell testing the correct - // coverprofile setting. Unfortunately this isn't sufficient because - // the testing oackage explicitly avoids calling flag.Parse again - // if flag.Parsed returns true, so we the coverprofile value directly - // too. - os.Args = []string{os.Args[0], "-test.coverprofile=" + cprof} - setCoverProfile(cprof) - - // Suppress the chatty coverage and test report. - devNull, err := os.Open(os.DevNull) - if err != nil { - panic(err) - } - os.Stdout = devNull - os.Stderr = devNull - - // Run MainStart (recursively, but it we should be ok) with no tests - // so that it writes the coverage profile. - m := mainStart() - if code := m.Run(); code != 0 && exitCode == 0 { - exitCode = code - } - if _, err := os.Stat(cprof); err != nil { - log.Printf("failed to write coverage profile %q", cprof) - } - if panicErr != nil { - // The error didn't originate from the flag package (we know that - // flag.PanicOnError causes an error value that implements error), - // so carry on panicking. - panic(panicErr) - } - }() - return mainf() -} - -func coverProfileFlag() flag.Getter { - f := flag.CommandLine.Lookup("test.coverprofile") - if f == nil { - // We've imported testing so it definitely should be there. - panic("cannot find test.coverprofile flag") - } - return f.Value.(flag.Getter) -} - -func coverProfile() string { - return coverProfileFlag().Get().(string) -} - -func setCoverProfile(cprof string) { - coverProfileFlag().Set(cprof) -} - type nopTestDeps struct{} func (nopTestDeps) MatchString(pat, str string) (result bool, err error) { diff --git a/testscript/testscript.go b/testscript/testscript.go index 00fcbf0f..31deecb0 100644 --- a/testscript/testscript.go +++ b/testscript/testscript.go @@ -427,7 +427,7 @@ func (ts *TestScript) setup() string { // 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"), + "GOCOVERDIR=" + os.Getenv("GOCOVERDIR"), }, WorkDir: ts.workdir, Values: make(map[interface{}]interface{}),