From 7a0799b2c0bdfaf745dbd8c74a3db2f3d238fd1b Mon Sep 17 00:00:00 2001 From: Dmitri Shuralyov Date: Wed, 8 Feb 2023 11:40:06 -0500 Subject: [PATCH] cmd/dist, test: convert test/run.go runner to a cmd/go test As motivated on the issue, we want to move the functionality of the run.go program to happen via a normal go test. Each .go test case in the GOROOT/test directory gets a subtest, and cmd/go's support for parallel test execution replaces run.go's own implementation thereof. The goal of this change is to have fairly minimal and readable diff while making an atomic changeover. The working directory is modified during the test execution to be GOROOT/test as it was with run.go, and most of the test struct and its run method are kept unchanged. The next CL in the stack applies further simplifications and cleanups that become viable. There's no noticeable difference in test execution time: it takes around 60-80 seconds both before and after on my machine. Test caching, which the previous runner lacked, can shorten the time significantly. For #37486. Fixes #56844. Change-Id: I209619dc9d90e7529624e49c01efeadfbeb5c9ae Reviewed-on: https://go-review.googlesource.com/c/go/+/463276 Run-TryBot: Dmitri Shuralyov Reviewed-by: Austin Clements Auto-Submit: Dmitri Shuralyov Reviewed-by: Dmitri Shuralyov TryBot-Result: Gopher Robot --- src/cmd/dist/test.go | 66 ++--- .../internal/testdir/testdir_test.go | 235 +++++------------- test/README.md | 4 +- test/codegen/README | 13 +- 4 files changed, 88 insertions(+), 230 deletions(-) rename test/run.go => src/internal/testdir/testdir_test.go (93%) diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go index bc58f0936b..a906c0dbdb 100644 --- a/src/cmd/dist/test.go +++ b/src/cmd/dist/test.go @@ -18,7 +18,6 @@ import ( "runtime" "strconv" "strings" - "sync" "time" ) @@ -594,6 +593,12 @@ func (t *tester) registerRaceBenchTest(pkg string) { } func (t *tester) registerTests() { + // registerStdTestSpecially tracks import paths in the standard library + // whose test registration happens in a special way. + registerStdTestSpecially := map[string]bool{ + "internal/testdir": true, // Registered at the bottom with sharding. + } + // Fast path to avoid the ~1 second of `go list std cmd` when // the caller lists specific tests to run. (as the continuous // build coordinator does). @@ -621,10 +626,16 @@ func (t *tester) registerTests() { } pkgs := strings.Fields(string(all)) for _, pkg := range pkgs { + if registerStdTestSpecially[pkg] { + continue + } t.registerStdTest(pkg) } if t.race { for _, pkg := range pkgs { + if registerStdTestSpecially[pkg] { + continue + } if t.packageHasBenchmarks(pkg) { t.registerRaceBenchTest(pkg) } @@ -907,12 +918,15 @@ func (t *tester) registerTests() { nShards = n } for shard := 0; shard < nShards; shard++ { - shard := shard - t.tests = append(t.tests, distTest{ - name: fmt.Sprintf("test:%d_%d", shard, nShards), - heading: "../test", - fn: func(dt *distTest) error { return t.testDirTest(dt, shard, nShards) }, - }) + t.registerTest( + fmt.Sprintf("test:%d_%d", shard, nShards), + "../test", + &goTest{ + dir: "internal/testdir", + testFlags: []string{fmt.Sprintf("-shard=%d", shard), fmt.Sprintf("-shards=%d", nShards)}, + }, + rtHostTest{}, + ) } } // Only run the API check on fast development platforms. @@ -1514,44 +1528,6 @@ func (t *tester) registerRaceTests() { } } -var runtest struct { - sync.Once - exe string - err error -} - -func (t *tester) testDirTest(dt *distTest, shard, shards int) error { - runtest.Do(func() { - f, err := os.CreateTemp("", "runtest-*.exe") // named exe for Windows, but harmless elsewhere - if err != nil { - runtest.err = err - return - } - f.Close() - - runtest.exe = f.Name() - xatexit(func() { - os.Remove(runtest.exe) - }) - - cmd := t.dirCmd("test", gorootBinGo, "build", "-o", runtest.exe, "run.go") - setEnv(cmd, "GOOS", gohostos) - setEnv(cmd, "GOARCH", gohostarch) - runtest.err = cmd.Run() - }) - if runtest.err != nil { - return runtest.err - } - if t.compileOnly { - return nil - } - t.addCmd(dt, "test", runtest.exe, - fmt.Sprintf("--shard=%d", shard), - fmt.Sprintf("--shards=%d", shards), - ) - return nil -} - // cgoPackages is the standard packages that use cgo. var cgoPackages = []string{ "net", diff --git a/test/run.go b/src/internal/testdir/testdir_test.go similarity index 93% rename from test/run.go rename to src/internal/testdir/testdir_test.go index 611fb02d72..f6b8e1da89 100644 --- a/test/run.go +++ b/src/internal/testdir/testdir_test.go @@ -1,11 +1,9 @@ -// skip - // Copyright 2012 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. -// Run runs tests in the test directory. -package main +// Package testdir_test runs tests in the GOROOT/test directory. +package testdir_test import ( "bytes" @@ -30,6 +28,7 @@ import ( "strconv" "strings" "sync" + "testing" "time" "unicode" ) @@ -37,10 +36,7 @@ import ( var ( verbose = flag.Bool("v", false, "verbose. if set, parallelism is set to 1.") keep = flag.Bool("k", false, "keep. keep temporary directory.") - numParallel = flag.Int("n", runtime.NumCPU(), "number of parallel tests to run") - summary = flag.Bool("summary", false, "show summary of results") allCodegen = flag.Bool("all_codegen", defaultAllCodeGen(), "run all goos/goarch for codegen") - showSkips = flag.Bool("show_skips", false, "show skipped tests") runSkips = flag.Bool("run_skips", false, "run skipped tests (ignore skip and build tags)") linkshared = flag.Bool("linkshared", false, "") updateErrors = flag.Bool("update_errors", false, "update error messages in test file based on compiler output") @@ -93,114 +89,45 @@ var ( // TODO(bradfitz): just use all directories? dirs = []string{".", "ken", "chan", "interface", "syntax", "dwarf", "fixedbugs", "codegen", "runtime", "abi", "typeparam", "typeparam/mdempsky"} - // ratec controls the max number of tests running at a time. - ratec chan bool - - // toRun is the channel of tests to run. - // It is nil until the first test is started. - toRun chan *test - // rungatec controls the max number of runoutput tests // executed in parallel as they can each consume a lot of memory. rungatec chan bool ) -// maxTests is an upper bound on the total number of tests. -// It is used as a channel buffer size to make sure sends don't block. -const maxTests = 5000 - -func main() { - flag.Parse() - - findExecCmd() - - // Disable parallelism if printing or if using a simulator. - if *verbose || len(findExecCmd()) > 0 { - *numParallel = 1 - *runoutputLimit = 1 +// Test is the main entrypoint that runs tests in the GOROOT/test directory. +// Each .go file test case in GOROOT/test is registered as a subtest. +func Test(t *testing.T) { + // TODO(go.dev/issue/56844): There's only a few subprocesses started, so it might be viable/safe enough to set cmd.Dir of subprocessess instead of doing it globally here. + err := os.Chdir("../../../test") + if err != nil { + t.Fatal(err) } + t.Cleanup(func() { os.Chdir("../src/internal/testdir") }) - ratec = make(chan bool, *numParallel) rungatec = make(chan bool, *runoutputLimit) - var tests []*test - if flag.NArg() > 0 { - for _, arg := range flag.Args() { - if arg == "-" || arg == "--" { - // Permit running: - // $ go run run.go - env.go - // $ go run run.go -- env.go - // $ go run run.go - ./fixedbugs - // $ go run run.go -- ./fixedbugs - continue - } - if fi, err := os.Stat(arg); err == nil && fi.IsDir() { - for _, baseGoFile := range goFiles(arg) { - tests = append(tests, startTest(arg, baseGoFile)) + for _, dir := range dirs { + for _, goFile := range goFiles(dir) { + test := test{dir: dir, gofile: goFile} + t.Run(path.Join(test.dir, test.gofile), func(t *testing.T) { + t.Parallel() + test.T = t + test.run() + if e, isSkip := test.err.(skipError); isSkip { + t.Fatal("unexpected skip:", e) } - } else if strings.HasSuffix(arg, ".go") { - dir, file := filepath.Split(arg) - tests = append(tests, startTest(dir, file)) - } else { - log.Fatalf("can't yet deal with non-directory and non-go file %q", arg) - } - } - } else { - for _, dir := range dirs { - for _, baseGoFile := range goFiles(dir) { - tests = append(tests, startTest(dir, baseGoFile)) - } - } - } - - failed := false - resCount := map[string]int{} - for _, test := range tests { - <-test.donec - status := "ok " - errStr := "" - if e, isSkip := test.err.(skipError); isSkip { - test.err = nil - errStr = "unexpected skip for " + path.Join(test.dir, test.gofile) + ": " + string(e) - status = "FAIL" - } - if test.err != nil { - errStr = test.err.Error() - if test.expectFail { - errStr += " (expected)" - } else { - status = "FAIL" - } - } else if test.expectFail { - status = "FAIL" - errStr = "unexpected success" - } - if status == "FAIL" { - failed = true - } - resCount[status]++ - dt := fmt.Sprintf("%.3fs", test.dt.Seconds()) - if status == "FAIL" { - fmt.Printf("# go run run.go -- %s\n%s\nFAIL\t%s\t%s\n", - path.Join(test.dir, test.gofile), - errStr, test.goFileName(), dt) - continue - } - if !*verbose { - continue - } - fmt.Printf("%s\t%s\t%s\n", status, test.goFileName(), dt) - } - - if *summary { - for k, v := range resCount { - fmt.Printf("%5d %s\n", v, k) + if test.err != nil { + if test.expectFail { + t.Log(test.err.Error() + " (expected)") + } else { + t.Fatal(test.err) + } + } else if test.expectFail { + t.Fatal("unexpected success") + } + }) } } - - if failed { - os.Exit(1) - } } // goTool reports the path of the go tool to use to run the tests. @@ -220,7 +147,7 @@ func goTool() string { } func shardMatch(name string) bool { - if *shards == 0 { + if *shards <= 1 { return true } h := fnv.New32() @@ -334,11 +261,9 @@ func (s skipError) Error() string { return string(s) } // test holds the state of a test. type test struct { - dir, gofile string - donec chan bool // closed when done - dt time.Duration + *testing.T - src string + dir, gofile string tempDir string err error @@ -375,39 +300,6 @@ func (t *test) initExpectFail() { } } -func startTest(dir, gofile string) *test { - t := &test{ - dir: dir, - gofile: gofile, - donec: make(chan bool, 1), - } - if toRun == nil { - toRun = make(chan *test, maxTests) - go runTests() - } - select { - case toRun <- t: - default: - panic("toRun buffer size (maxTests) is too small") - } - return t -} - -// runTests runs tests in parallel, but respecting the order they -// were enqueued on the toRun channel. -func runTests() { - for { - ratec <- true - t := <-toRun - go func() { - t.run() - <-ratec - }() - } -} - -var cwd, _ = os.Getwd() - func (t *test) goFileName() string { return filepath.Join(t.dir, t.gofile) } @@ -550,10 +442,6 @@ func (ctxt *context) match(name string) bool { return false } -func init() { - checkShouldTest() -} - // goGcflags returns the -gcflags argument to use with go build / go run. // This must match the flags used for building the standard library, // or else the commands will rebuild any needed packages (like runtime) @@ -570,25 +458,18 @@ var errTimeout = errors.New("command exceeded time limit") // run runs a test. func (t *test) run() { - start := time.Now() - defer func() { - t.dt = time.Since(start) - close(t.donec) - }() - srcBytes, err := ioutil.ReadFile(t.goFileName()) if err != nil { t.err = err return - } - t.src = string(srcBytes) - if t.src[0] == '\n' { + } else if bytes.HasPrefix(srcBytes, []byte{'\n'}) { t.err = skipError("starts with newline") return } + src := string(srcBytes) // Execution recipe stops at first blank line. - action, _, ok := strings.Cut(t.src, "\n\n") + action, _, ok := strings.Cut(src, "\n\n") if !ok { t.err = fmt.Errorf("double newline ending execution recipe not found in %s", t.goFileName()) return @@ -600,15 +481,12 @@ func (t *test) run() { action = strings.TrimPrefix(action, "//") // Check for build constraints only up to the actual code. - header, _, ok := strings.Cut(t.src, "\npackage") + header, _, ok := strings.Cut(src, "\npackage") if !ok { header = action // some files are intentionally malformed } if ok, why := shouldTest(header, goos, goarch); !ok { - if *showSkips { - fmt.Printf("%-20s %-20s: %s\n", "skip", t.goFileName(), why) - } - return + t.Skip(why) } var args, flags, runenv []string @@ -642,7 +520,7 @@ func (t *test) run() { if *runSkips { break } - return + t.Skip("skip") default: t.err = skipError("skipped; unknown pattern: " + action) return @@ -806,6 +684,10 @@ func (t *test) run() { return filename } + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } long := filepath.Join(cwd, t.goFileName()) switch action { default: @@ -1284,20 +1166,17 @@ func (t *test) run() { } } +var execCmdOnce sync.Once var execCmd []string func findExecCmd() []string { - if execCmd != nil { - return execCmd - } - execCmd = []string{} // avoid work the second time - if goos == runtime.GOOS && goarch == runtime.GOARCH { - return execCmd - } - path, err := exec.LookPath(fmt.Sprintf("go_%s_%s_exec", goos, goarch)) - if err == nil { - execCmd = []string{path} - } + execCmdOnce.Do(func() { + if goos == runtime.GOOS && goarch == runtime.GOARCH { + // Do nothing. + } else if path, err := exec.LookPath(fmt.Sprintf("go_%s_%s_exec", goos, goarch)); err == nil { + execCmd = []string{path} + } + }) return execCmd } @@ -1904,14 +1783,18 @@ func defaultRunOutputLimit() int { return cpu } -// checkShouldTest runs sanity checks on the shouldTest function. -func checkShouldTest() { +func TestShouldTest(t *testing.T) { + if *shard != 0 { + t.Skipf("nothing to test on shard index %d", *shard) + } + assert := func(ok bool, _ string) { + t.Helper() if !ok { - panic("fail") + t.Error("test case failed") } } - assertNot := func(ok bool, _ string) { assert(!ok, "") } + assertNot := func(ok bool, _ string) { t.Helper(); assert(!ok, "") } // Simple tests. assert(shouldTest("// +build linux", "linux", "arm")) diff --git a/test/README.md b/test/README.md index 432d36b653..7e3948f947 100644 --- a/test/README.md +++ b/test/README.md @@ -4,11 +4,11 @@ They are run as part of all.bash. To run just these tests, execute: - ../bin/go run run.go + ../bin/go test internal/testdir To run just tests from specified files in this directory, execute: - ../bin/go run run.go -- file1.go file2.go ... + ../bin/go test internal/testdir -run='Test/(file1.go|file2.go|...)' Standard library tests should be written as regular Go tests in the appropriate package. diff --git a/test/codegen/README b/test/codegen/README index b803fe585f..1d68780394 100644 --- a/test/codegen/README +++ b/test/codegen/README @@ -11,23 +11,22 @@ compiler. The test harness compiles Go code inside files in this directory and matches the generated assembly (the output of `go tool compile -S`) against a set of regexps to be specified in comments that follow a -special syntax (described below). The test driver is implemented as a -step of the top-level test/run.go suite, called "asmcheck". +special syntax (described below). The test driver is implemented as +an action within the GOROOT/test test suite, called "asmcheck". The codegen harness is part of the all.bash test suite, but for performance reasons only the codegen tests for the host machine's GOARCH are enabled by default, and only on GOOS=linux. To perform comprehensive tests for all the supported architectures -(even on a non-Linux system), one can run the following command +(even on a non-Linux system), one can run the following command: - $ ../bin/go run run.go -all_codegen -v codegen + $ ../../bin/go test internal/testdir -run='Test/codegen' -all_codegen -v -in the top-level test directory. This is recommended after any change -that affect the compiler's code. +This is recommended after any change that affect the compiler's code. The test harness compiles the tests with the same go toolchain that is -used to run run.go. After writing tests for a newly added codegen +used to run the test. After writing tests for a newly added codegen transformation, it can be useful to first run the test harness with a toolchain from a released Go version (and verify that the new tests fail), and then re-runnig the tests using the devel toolchain. -- 2.48.1