From 509766756d85fb2835aa472a2c998afa3249dc0a Mon Sep 17 00:00:00 2001 From: Chris Novakovic Date: Tue, 27 Aug 2024 13:12:26 +0100 Subject: [PATCH] please_go: remove Go <= 1.19 coverage generation code Go 1.20 made breaking changes to the way in which coverage is computed. This required the `coverageredesign` GOEXPERIMENT to be enabled when using go-rules with Go 1.20 onwards. Go 1.19 has now been EOL for almost a year, and the `coveragedesign` GOEXPERIMENT will be removed from Go shortly, having been the default for around 18 months. Remove the following features from please_go that supported the Go <= 1.19 method of computing coverage: * The `covervars` command, which is no longer necessary with `coverageredesign`. * The generation of Go <= 1.19-compatible test main files with the `testmain` command. --- tools/please_go/BUILD | 2 - tools/please_go/covervars/BUILD | 15 -- tools/please_go/covervars/cover_vars.go | 25 ---- tools/please_go/please_go.go | 11 -- tools/please_go/test/BUILD | 13 -- tools/please_go/test/find_cover_vars.go | 94 ------------ tools/please_go/test/find_cover_vars_test.go | 45 ------ tools/please_go/test/gotest.go | 19 +-- .../test/test_data/test/example_test.go | 21 --- tools/please_go/test/write_test_main.go | 141 +----------------- tools/please_go/test/write_test_main_test.go | 36 +---- 11 files changed, 6 insertions(+), 416 deletions(-) delete mode 100644 tools/please_go/covervars/BUILD delete mode 100644 tools/please_go/covervars/cover_vars.go delete mode 100644 tools/please_go/test/find_cover_vars.go delete mode 100644 tools/please_go/test/find_cover_vars_test.go diff --git a/tools/please_go/BUILD b/tools/please_go/BUILD index 6f386ab8..a65616b6 100644 --- a/tools/please_go/BUILD +++ b/tools/please_go/BUILD @@ -10,7 +10,6 @@ go_binary( deps = [ "//third_party/go:flags", "//tools/please_go/cover", - "//tools/please_go/covervars", "//tools/please_go/embed", "//tools/please_go/filter", "//tools/please_go/generate", @@ -38,7 +37,6 @@ genrule( visibility = ["PUBLIC"], deps = [ "//tools/please_go/cover:srcs", - "//tools/please_go/covervars:srcs", "//tools/please_go/embed:srcs", "//tools/please_go/filter:srcs", "//tools/please_go/generate:srcs", diff --git a/tools/please_go/covervars/BUILD b/tools/please_go/covervars/BUILD deleted file mode 100644 index 3a1aba1c..00000000 --- a/tools/please_go/covervars/BUILD +++ /dev/null @@ -1,15 +0,0 @@ -subinclude("//build_defs:go") - -filegroup( - name = "srcs", - srcs = glob(["*.go"]), - visibility = ["//tools/please_go:bootstrap"], -) - -go_library( - name = "covervars", - srcs = [ - "cover_vars.go", - ], - visibility = ["PUBLIC"], -) diff --git a/tools/please_go/covervars/cover_vars.go b/tools/please_go/covervars/cover_vars.go deleted file mode 100644 index 1f51e446..00000000 --- a/tools/please_go/covervars/cover_vars.go +++ /dev/null @@ -1,25 +0,0 @@ -package covervars - -import ( - "fmt" - "io" - "log" - "path/filepath" - "strings" -) - -// GenCoverVars writes the coverage variable information to w -func GenCoverVars(w io.Writer, importPath string, srcs []string) { - for _, src := range srcs { - if _, err := w.Write([]byte(coverVar(src, importPath))); err != nil { - log.Fatalf("%v", err) - } - } -} - -func coverVar(src, importPath string) string { - baseName := filepath.Base(src) - varname := strings.ReplaceAll(baseName, ".", "_") - varname = strings.ReplaceAll(varname, "-", "_") - return fmt.Sprintf("%s=GoCover_%s\n", importPath, varname) -} diff --git a/tools/please_go/please_go.go b/tools/please_go/please_go.go index 1f7b1b52..25500f0a 100644 --- a/tools/please_go/please_go.go +++ b/tools/please_go/please_go.go @@ -10,7 +10,6 @@ import ( "github.com/peterebden/go-cli-init/v5/flags" "github.com/please-build/go-rules/tools/please_go/cover" - "github.com/please-build/go-rules/tools/please_go/covervars" "github.com/please-build/go-rules/tools/please_go/embed" "github.com/please-build/go-rules/tools/please_go/filter" "github.com/please-build/go-rules/tools/please_go/generate" @@ -52,12 +51,6 @@ var opts = struct { Sources []string `positional-arg-name:"sources" description:"Test source files" required:"true"` } `positional-args:"true" required:"true"` } `command:"testmain" alias:"t" description:"Generates a go main package to run the tests in a package."` - CoverVars struct { - ImportPath string `short:"i" long:"import_path" description:"The import path for the source files"` - Args struct { - Sources []string `positional-arg-name:"sources" description:"Source files to generate embed config for"` - } `positional-args:"true"` - } `command:"covervars" description:"Generates coverage variable config for a set of go src files"` Cover struct { GoTool string `short:"g" long:"go" default:"go" env:"TOOLS_GO" description:"Go binary to run"` CoverTool string `short:"t" long:"go_cover" env:"TOOLS_COVER" description:"Go coverage tool to run"` @@ -170,10 +163,6 @@ var subCommands = map[string]func() int{ } return 0 }, - "covervars": func() int { - covervars.GenCoverVars(os.Stdout, opts.CoverVars.ImportPath, opts.CoverVars.Args.Sources) - return 0 - }, "filter": func() int { filter.Filter(opts.Filter.Tags, opts.Filter.Args.Sources) return 0 diff --git a/tools/please_go/test/BUILD b/tools/please_go/test/BUILD index f0eba924..4920b927 100644 --- a/tools/please_go/test/BUILD +++ b/tools/please_go/test/BUILD @@ -9,7 +9,6 @@ filegroup( go_library( name = "test", srcs = [ - "find_cover_vars.go", "gotest.go", "write_test_main.go", ], @@ -19,18 +18,6 @@ go_library( ], ) -go_test( - name = "find_cover_vars_test", - srcs = ["find_cover_vars_test.go"], - data = [ - "test_data", - ], - deps = [ - ":test", - "//third_party/go:testify", - ], -) - go_test( name = "write_test_main_test", srcs = ["write_test_main_test.go"], diff --git a/tools/please_go/test/find_cover_vars.go b/tools/please_go/test/find_cover_vars.go deleted file mode 100644 index 893f3d4d..00000000 --- a/tools/please_go/test/find_cover_vars.go +++ /dev/null @@ -1,94 +0,0 @@ -// Package test contains utilities used by plz_go_test. -package test - -import ( - "fmt" - "os" - "path" - "path/filepath" - "strings" -) - -// A CoverVar is just a combination of package path and variable name -// for one of the templated-in coverage variables. -type CoverVar struct { - Dir, ImportPath, ImportName, Var, File string -} - -// FindCoverVars searches the given directory recursively to find all Go files with coverage variables. -func FindCoverVars(dir, testPackage string, external bool, excludedDirs []string) ([]CoverVar, error) { - if dir == "" { - return nil, nil - } - excludeMap := map[string]struct{}{} - for _, e := range excludedDirs { - excludeMap[e] = struct{}{} - } - var ret []CoverVar - - err := filepath.WalkDir(dir, func(name string, info os.DirEntry, err error) error { - if err != nil { - return err - } - if _, present := excludeMap[name]; present { - if info.IsDir() { - return filepath.SkipDir - } - } else if strings.HasSuffix(name, ".cover_vars") { - vars, err := parseCoverVars(name, testPackage, external) - if err != nil { - return err - } - ret = append(ret, vars...) - } - return nil - }) - return ret, err -} - -// parseCoverVars parses the coverage variables file for all cover vars -func parseCoverVars(filepath, testPackage string, external bool) ([]CoverVar, error) { - dir := strings.TrimRight(path.Dir(filepath), "/") - if dir == "" { - dir = "." - } - - b, err := os.ReadFile(filepath) - if err != nil { - return nil, err - } - lines := strings.Split(string(b), "\n") - ret := make([]CoverVar, 0, len(lines)) - for _, line := range lines { - if strings.TrimSpace(line) == "" { - continue - } - - parts := strings.Split(line, "=") - if len(parts) != 2 { - return nil, fmt.Errorf("malformed cover var line in %v: %v", filepath, line) - } - if external || dir != os.Getenv("PKG_DIR") { - ret = append(ret, coverVar(dir, parts[0], parts[1])) - } else { - // We recompile the test package, so override the import path here to get the right version - ret = append(ret, coverVar(dir, testPackage, parts[1])) - } - } - - return ret, nil -} - -func coverVar(dir, importPath, v string) CoverVar { - fmt.Fprintf(os.Stderr, "Found cover variable: %s %s %s\n", dir, importPath, v) - f := path.Join(dir, strings.TrimPrefix(v, "GoCover_")) - if strings.HasSuffix(f, "_go") { - f = f[:len(f)-3] + ".go" - } - return CoverVar{ - Dir: dir, - ImportPath: importPath, - Var: v, - File: f, - } -} diff --git a/tools/please_go/test/find_cover_vars_test.go b/tools/please_go/test/find_cover_vars_test.go deleted file mode 100644 index c4a2512f..00000000 --- a/tools/please_go/test/find_cover_vars_test.go +++ /dev/null @@ -1,45 +0,0 @@ -package test - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -// The library here is a (very) reduced version of core that only has one file in it. -var coverageVars = []CoverVar{{ - Dir: "tools/please_go/test/test_data", - ImportPath: "tools/please_go/test/test_data/core", - Var: "GoCover_lock_go", - File: "tools/please_go/test/test_data/lock.go", -}} - -func TestFindCoverVars(t *testing.T) { - vars, err := FindCoverVars("tools/please_go/test/test_data", "", false, []string{"tools/please_go/test/test_data/x", "tools/please_go/test/test_data/binary"}) - assert.NoError(t, err) - assert.Equal(t, coverageVars, vars) -} - -func TestFindCoverVarsFailsGracefully(t *testing.T) { - _, err := FindCoverVars("wibble", "", false, nil) - assert.Error(t, err) -} - -func TestFindCoverVarsReturnsNothingForEmptyPath(t *testing.T) { - vars, err := FindCoverVars("", "", false, nil) - assert.NoError(t, err) - assert.Equal(t, 0, len(vars)) -} - -func TestFindBinaryCoverVars(t *testing.T) { - // Test for Go 1.7 binary format. - expected := []CoverVar{{ - Dir: "tools/please_go/test/test_data/binary", - ImportPath: "tools/please_go/test/test_data/binary/core", - Var: "GoCover_lock_go", - File: "tools/please_go/test/test_data/binary/lock.go", - }} - vars, err := FindCoverVars("tools/please_go/test/test_data/binary", "", false, nil) - assert.NoError(t, err) - assert.Equal(t, expected, vars) -} diff --git a/tools/please_go/test/gotest.go b/tools/please_go/test/gotest.go index f1e1f855..ddf16960 100644 --- a/tools/please_go/test/gotest.go +++ b/tools/please_go/test/gotest.go @@ -2,28 +2,11 @@ package test import ( "log" - - "github.com/please-build/go-rules/tools/please_go/install/toolchain" ) // PleaseGoTest will generate the test main for the provided sources func PleaseGoTest(goTool, dir, testPackage, output string, sources, exclude []string, isBenchmark, external bool) { - tc := toolchain.Toolchain{GoTool: goTool} - minor, err := tc.GoMinorVersion() - if err != nil { - log.Fatalf("Failed to determine Go version: %s", err) - } - if minor >= 20 { - if err := WriteTestMain(testPackage, sources, output, dir != "", nil, isBenchmark, true, true); err != nil { - log.Fatalf("Error writing test main: %s", err) - } - return - } - coverVars, err := FindCoverVars(dir, testPackage, external, exclude) - if err != nil { - log.Fatalf("Error scanning for coverage: %s", err) - } - if err := WriteTestMain(testPackage, sources, output, dir != "", coverVars, isBenchmark, minor >= 18, false); err != nil { + if err := WriteTestMain(testPackage, sources, output, dir != "", isBenchmark, true); err != nil { log.Fatalf("Error writing test main: %s", err) } } diff --git a/tools/please_go/test/test_data/test/example_test.go b/tools/please_go/test/test_data/test/example_test.go index 102f6d00..9c868730 100644 --- a/tools/please_go/test/test_data/test/example_test.go +++ b/tools/please_go/test/test_data/test/example_test.go @@ -24,27 +24,6 @@ func TestReadCopiedPkgdef(t *testing.T) { assert.Equal(t, coverageVars, vars) } -func TestFindCoverVars(t *testing.T) { - vars, err := FindCoverVars("src/build/go/test_data", []string{"src/build/go/test_data/x"}) - assert.NoError(t, err) - assert.Equal(t, coverageVars, vars) -} - -func TestFindCoverVarsFailsGracefully(t *testing.T) { - _, err := FindCoverVars("wibble", []string{}) - assert.Error(t, err) -} - -func TestFindCoverVarsReturnsNothingForEmptyPath(t *testing.T) { - vars, err := FindCoverVars("", []string{}) - assert.NoError(t, err) - assert.Equal(t, 0, len(vars)) -} - func readPkgdef(name string) ([]string, error) { return nil, nil } - -func FindCoverVars(filename string, vars []string) ([]string, error) { - return nil, nil -} diff --git a/tools/please_go/test/write_test_main.go b/tools/please_go/test/write_test_main.go index f19450a4..dbec758f 100644 --- a/tools/please_go/test/write_test_main.go +++ b/tools/please_go/test/write_test_main.go @@ -20,7 +20,6 @@ type testDescr struct { BenchFunctions []string FuzzFunctions []string Examples []*doc.Example - CoverVars []CoverVar Imports []string Coverage bool Benchmark bool @@ -28,20 +27,14 @@ type testDescr struct { } // WriteTestMain templates a test main file from the given sources to the given output file. -func WriteTestMain(testPackage string, sources []string, output string, coverage bool, coverVars []CoverVar, benchmark, hasFuzz, coverageRedesign bool) error { +func WriteTestMain(testPackage string, sources []string, output string, coverage, benchmark, hasFuzz bool) error { testDescr, err := parseTestSources(sources) if err != nil { return err } testDescr.Coverage = coverage - testDescr.CoverVars = coverVars if len(testDescr.TestFunctions) > 0 || len(testDescr.BenchFunctions) > 0 || len(testDescr.Examples) > 0 || len(testDescr.FuzzFunctions) > 0 || testDescr.Main != "" { - // Can't set this if there are no test functions, it'll be an unused import. - if coverageRedesign { testDescr.Imports = []string{fmt.Sprintf("%s \"%s\"", testDescr.Package, testPackage)} - } else { - testDescr.Imports = extraImportPaths(testPackage, testDescr.Package, testDescr.CoverVars) - } } testDescr.Benchmark = benchmark @@ -55,22 +48,7 @@ func WriteTestMain(testPackage string, sources []string, output string, coverage // This might be consumed by other things. fmt.Printf("Package: %s\n", testDescr.Package) - if coverageRedesign { - return testMainTmpl.Execute(f, testDescr) - } - return oldTestMainTmpl.Execute(f, testDescr) -} - -func extraImportPaths(testPackage, alias string, coverVars []CoverVar) []string { - ret := make([]string, 0, len(coverVars)+1) - ret = append(ret, fmt.Sprintf("%s \"%s\"", alias, testPackage)) - - for i, v := range coverVars { - name := fmt.Sprintf("_cover%d", i) - coverVars[i].ImportName = name - ret = append(ret, fmt.Sprintf("%s \"%s\"", name, v.ImportPath)) - } - return ret + return testMainTmpl.Execute(f, testDescr) } // parseTestSources parses the test sources and returns the package and set of test functions in them. @@ -253,118 +231,3 @@ func main() { _gostdlib_os.Exit(internalMain()) } `)) - -var oldTestMainTmpl = template.Must(template.New("oldmain").Parse(` -package main - -import ( - _gostdlib_os "os" - {{if not .Benchmark}}_gostdlib_strings "strings"{{end}} - _gostdlib_testing "testing" - _gostdlib_testdeps "testing/internal/testdeps" - -{{range .Imports}} - {{.}} -{{end}} -) - -var tests = []_gostdlib_testing.InternalTest{ -{{range .TestFunctions}} - {"{{.}}", {{$.Package}}.{{.}}}, -{{end}} -} -var examples = []_gostdlib_testing.InternalExample{ -{{range .Examples}} - {"{{.Name}}", {{$.Package}}.Example{{.Name}}, {{.Output | printf "%q"}}, {{.Unordered}}}, -{{end}} -} - -var benchmarks = []_gostdlib_testing.InternalBenchmark{ -{{range .BenchFunctions}} - {"{{.}}", {{$.Package}}.{{.}}}, -{{end}} -} - -{{ if .HasFuzz }} -var fuzzTargets = []_gostdlib_testing.InternalFuzzTarget{ -{{ range .FuzzFunctions }} - {"{{.}}", {{$.Package}}.{{.}}}, -{{ end }} -} -{{ end }} - -{{if .Coverage}} - -// Only updated by init functions, so no need for atomicity. -var ( - coverCounters = make(map[string][]uint32) - coverBlocks = make(map[string][]_gostdlib_testing.CoverBlock) -) - -func init() { - {{range $i, $c := .CoverVars}} - {{if $c.ImportName }} - coverRegisterFile({{printf "%q" $c.File}}, {{$c.ImportName}}.{{$c.Var}}.Count[:], {{$c.ImportName}}.{{$c.Var}}.Pos[:], {{$c.ImportName}}.{{$c.Var}}.NumStmt[:]) - {{end}} - {{end}} -} - -func coverRegisterFile(fileName string, counter []uint32, pos []uint32, numStmts []uint16) { - if 3*len(counter) != len(pos) || len(counter) != len(numStmts) { - panic("coverage: mismatched sizes") - } - if coverCounters[fileName] != nil { - // Already registered. - return - } - coverCounters[fileName] = counter - block := make([]_gostdlib_testing.CoverBlock, len(counter)) - for i := range counter { - block[i] = _gostdlib_testing.CoverBlock{ - Line0: pos[3*i+0], - Col0: uint16(pos[3*i+2]), - Line1: pos[3*i+1], - Col1: uint16(pos[3*i+2]>>16), - Stmts: numStmts[i], - } - } - coverBlocks[fileName] = block -} -{{end}} - -var testDeps = _gostdlib_testdeps.TestDeps{} - -func main() { -{{if .Coverage}} - _gostdlib_testing.RegisterCover(_gostdlib_testing.Cover{ - Mode: "set", - Counters: coverCounters, - Blocks: coverBlocks, - CoveredPackages: "", - }) - coverfile := _gostdlib_os.Getenv("COVERAGE_FILE") - args := []string{_gostdlib_os.Args[0], "-test.v", "-test.coverprofile", coverfile} -{{else}} - args := []string{_gostdlib_os.Args[0], "-test.v"} -{{end}} -{{if not .Benchmark}} - testVar := _gostdlib_os.Getenv("TESTS") - if testVar != "" { - testVar = _gostdlib_strings.ReplaceAll(testVar, " ", "|") - args = append(args, "-test.run", testVar) - } - _gostdlib_os.Args = append(args, _gostdlib_os.Args[1:]...) - m := _gostdlib_testing.MainStart(testDeps, tests, nil,{{ if .HasFuzz }} fuzzTargets,{{ end }} examples) -{{else}} - args = append(args, "-test.bench", ".*") - _gostdlib_os.Args = append(args, _gostdlib_os.Args[1:]...) - m := _gostdlib_testing.MainStart(testDeps, nil, benchmarks,{{ if .HasFuzz }} fuzzTargets,{{ end }} nil) -{{end}} - -{{if .Main}} - {{.Package}}.{{.Main}}(m) -{{else}} - _gostdlib_os.Exit(m.Run()) -{{end}} -} -`)) diff --git a/tools/please_go/test/write_test_main_test.go b/tools/please_go/test/write_test_main_test.go index 1bcc5099..523d1bb2 100644 --- a/tools/please_go/test/write_test_main_test.go +++ b/tools/please_go/test/write_test_main_test.go @@ -17,9 +17,6 @@ func TestParseTestSources(t *testing.T) { functions := []string{ "TestReadPkgdef", "TestReadCopiedPkgdef", - "TestFindCoverVars", - "TestFindCoverVarsFailsGracefully", - "TestFindCoverVarsReturnsNothingForEmptyPath", } assert.Equal(t, functions, descr.TestFunctions) } @@ -47,7 +44,7 @@ func TestParseTestSourcesFailsGracefully(t *testing.T) { } func TestWriteTestMain(t *testing.T) { - err := WriteTestMain("test_pkg", []string{"tools/please_go/test/test_data/test/example_test.go"}, "test.go", false, []CoverVar{}, false, true, false) + err := WriteTestMain("test_pkg", []string{"tools/please_go/test/test_data/test/example_test.go"}, "test.go", false, false, true) assert.NoError(t, err) // It's not really practical to assert the contents of the file in great detail. // We'll do the obvious thing of asserting that it is valid Go source. @@ -57,12 +54,7 @@ func TestWriteTestMain(t *testing.T) { } func TestWriteTestMainWithCoverage(t *testing.T) { - err := WriteTestMain("test_package", []string{"tools/please_go/test/test_data/test/example_test.go"}, "test.go", true, []CoverVar{{ - Dir: "tools/please_go/test/test_data", - ImportPath: "core", - Var: "GoCover_lock_go", - File: "tools/please_go/test/test_data/lock.go", - }}, false, false, false) + err := WriteTestMain("test_package", []string{"tools/please_go/test/test_data/test/example_test.go"}, "test.go", true, false, false) assert.NoError(t, err) // It's not really practical to assert the contents of the file in great detail. // We'll do the obvious thing of asserting that it is valid Go source. @@ -72,7 +64,7 @@ func TestWriteTestMainWithCoverage(t *testing.T) { } func TestWriteTestMainWithBenchmark(t *testing.T) { - err := WriteTestMain("test_package", []string{"tools/please_go/test/test_data/bench/example_benchmark_test.go"}, "test.go", true, []CoverVar{}, true, true, false) + err := WriteTestMain("test_package", []string{"tools/please_go/test/test_data/bench/example_benchmark_test.go"}, "test.go", true, true, true) assert.NoError(t, err) // It's not really practical to assert the contents of the file in great detail. // We'll do the obvious thing of asserting that it is valid Go source. @@ -84,25 +76,3 @@ func TestWriteTestMainWithBenchmark(t *testing.T) { assert.NoError(t, err) assert.Contains(t, string(test), "BenchmarkExample") } - -func TestExtraImportPaths(t *testing.T) { - assert.Equal(t, extraImportPaths("core", "core", []CoverVar{ - {ImportPath: "core"}, - {ImportPath: "output"}, - }), []string{ - "core \"core\"", - "_cover0 \"core\"", - "_cover1 \"output\"", - }) -} - -func TestExtraImportPathsWithImportPath(t *testing.T) { - assert.Equal(t, extraImportPaths("core", "core", []CoverVar{ - {ImportPath: "github.com/thought-machine/please/src/core"}, - {ImportPath: "github.com/thought-machine/please/output"}, - }), []string{ - "core \"core\"", - "_cover0 \"github.com/thought-machine/please/src/core\"", - "_cover1 \"github.com/thought-machine/please/output\"", - }) -}