From b978661c6cbaacda12e290b1e88c7675e49452dd Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 11 Jan 2022 22:39:53 -0500 Subject: [PATCH] cmd/godoc: streamline test subprocesses - Use testenv.Command to obtain Cmd instances that terminate (with useful goroutine dumps!) before the test's timeout, and remove arbitrary hard-coded timeouts. - Execute the test binary itself as cmd/godoc instead of invoking (and cleaning up after) 'go build'. - Use context cancellation to reduce the number of ad-hoc goroutines and channels needed by the tests and to provide stronger invariants on process cleanup. For golang/go#50014 Change-Id: I19ae4d10da691db233c79734799ae074ffdf6a03 Reviewed-on: https://go-review.googlesource.com/c/tools/+/377836 Run-TryBot: Bryan Mills Reviewed-by: Joedian Reid Reviewed-by: Dmitri Shuralyov TryBot-Result: Gopher Robot gopls-CI: kokoro Auto-Submit: Bryan Mills --- cmd/godoc/godoc_test.go | 189 ++++++++++++++++++---------------------- 1 file changed, 87 insertions(+), 102 deletions(-) diff --git a/cmd/godoc/godoc_test.go b/cmd/godoc/godoc_test.go index 4eb341af1e1..3e91ac6f94c 100644 --- a/cmd/godoc/godoc_test.go +++ b/cmd/godoc/godoc_test.go @@ -2,10 +2,11 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -package main_test +package main import ( "bytes" + "context" "fmt" "go/build" "io/ioutil" @@ -13,10 +14,10 @@ import ( "net/http" "os" "os/exec" - "path/filepath" "regexp" "runtime" "strings" + "sync" "testing" "time" @@ -24,42 +25,39 @@ import ( "golang.org/x/tools/internal/testenv" ) -// buildGodoc builds the godoc executable. -// It returns its path, and a cleanup function. -// -// TODO(adonovan): opt: do this at most once, and do the cleanup -// exactly once. How though? There's no atexit. -func buildGodoc(t *testing.T) (bin string, cleanup func()) { - t.Helper() - - if runtime.GOARCH == "arm" { - t.Skip("skipping test on arm platforms; too slow") - } - if runtime.GOOS == "android" { - t.Skipf("the dependencies are not available on android") +func TestMain(m *testing.M) { + if os.Getenv("GODOC_TEST_IS_GODOC") != "" { + main() + os.Exit(0) } - testenv.NeedsTool(t, "go") - tmp, err := ioutil.TempDir("", "godoc-regtest-") - if err != nil { - t.Fatal(err) - } - defer func() { - if cleanup == nil { // probably, go build failed. - os.RemoveAll(tmp) - } - }() + // Inform subprocesses that they should run the cmd/godoc main instead of + // running tests. It's a close approximation to building and running the real + // command, and much less complicated and expensive to build and clean up. + os.Setenv("GODOC_TEST_IS_GODOC", "1") - bin = filepath.Join(tmp, "godoc") - if runtime.GOOS == "windows" { - bin += ".exe" - } - cmd := exec.Command("go", "build", "-o", bin) - if err := cmd.Run(); err != nil { - t.Fatalf("Building godoc: %v", err) + os.Exit(m.Run()) +} + +var exe struct { + path string + err error + once sync.Once +} + +func godocPath(t *testing.T) string { + switch runtime.GOOS { + case "js", "ios": + t.Skipf("skipping test that requires exec") } - return bin, func() { os.RemoveAll(tmp) } + exe.once.Do(func() { + exe.path, exe.err = os.Executable() + }) + if exe.err != nil { + t.Fatal(exe.err) + } + return exe.path } func serverAddress(t *testing.T) string { @@ -74,60 +72,42 @@ func serverAddress(t *testing.T) string { return ln.Addr().String() } -func waitForServerReady(t *testing.T, cmd *exec.Cmd, addr string) { - ch := make(chan error, 1) - go func() { ch <- fmt.Errorf("server exited early: %v", cmd.Wait()) }() - go waitForServer(t, ch, +func waitForServerReady(t *testing.T, ctx context.Context, cmd *exec.Cmd, addr string) { + waitForServer(t, ctx, fmt.Sprintf("http://%v/", addr), "Go Documentation Server", - 15*time.Second, false) - if err := <-ch; err != nil { - t.Skipf("skipping due to https://go.dev/issue/50014: %v", err) - } } -func waitForSearchReady(t *testing.T, cmd *exec.Cmd, addr string) { - ch := make(chan error, 1) - go func() { ch <- fmt.Errorf("server exited early: %v", cmd.Wait()) }() - go waitForServer(t, ch, +func waitForSearchReady(t *testing.T, ctx context.Context, cmd *exec.Cmd, addr string) { + waitForServer(t, ctx, fmt.Sprintf("http://%v/search?q=FALLTHROUGH", addr), "The list of tokens.", - 2*time.Minute, false) - if err := <-ch; err != nil { - t.Skipf("skipping due to https://go.dev/issue/50014: %v", err) - } } -func waitUntilScanComplete(t *testing.T, addr string) { - ch := make(chan error) - go waitForServer(t, ch, +func waitUntilScanComplete(t *testing.T, ctx context.Context, addr string) { + waitForServer(t, ctx, fmt.Sprintf("http://%v/pkg", addr), "Scan is not yet complete", - 2*time.Minute, // setting reverse as true, which means this waits // until the string is not returned in the response anymore - true, - ) - if err := <-ch; err != nil { - t.Skipf("skipping due to https://go.dev/issue/50014: %v", err) - } + true) } -const pollInterval = 200 * time.Millisecond +const pollInterval = 50 * time.Millisecond -// waitForServer waits for server to meet the required condition. -// It sends a single error value to ch, unless the test has failed. -// The error value is nil if the required condition was met within -// timeout, or non-nil otherwise. -func waitForServer(t *testing.T, ch chan<- error, url, match string, timeout time.Duration, reverse bool) { - deadline := time.Now().Add(timeout) - for time.Now().Before(deadline) { - time.Sleep(pollInterval) - if t.Failed() { - return +// waitForServer waits for server to meet the required condition, +// failing the test if ctx is canceled before that occurs. +func waitForServer(t *testing.T, ctx context.Context, url, match string, reverse bool) { + start := time.Now() + for { + if ctx.Err() != nil { + t.Helper() + t.Fatalf("server failed to respond in %v", time.Since(start)) } + + time.Sleep(pollInterval) res, err := http.Get(url) if err != nil { continue @@ -140,11 +120,9 @@ func waitForServer(t *testing.T, ch chan<- error, url, match string, timeout tim switch { case !reverse && bytes.Contains(body, []byte(match)), reverse && !bytes.Contains(body, []byte(match)): - ch <- nil return } } - ch <- fmt.Errorf("server failed to respond in %v", timeout) } // hasTag checks whether a given release tag is contained in the current version @@ -158,24 +136,18 @@ func hasTag(t string) bool { return false } -func killAndWait(cmd *exec.Cmd) { - cmd.Process.Kill() - cmd.Process.Wait() -} - func TestURL(t *testing.T) { if runtime.GOOS == "plan9" { t.Skip("skipping on plan9; fails to start up quickly enough") } - bin, cleanup := buildGodoc(t) - defer cleanup() + bin := godocPath(t) testcase := func(url string, contents string) func(t *testing.T) { return func(t *testing.T) { stdout, stderr := new(bytes.Buffer), new(bytes.Buffer) args := []string{fmt.Sprintf("-url=%s", url)} - cmd := exec.Command(bin, args...) + cmd := testenv.Command(t, bin, args...) cmd.Stdout = stdout cmd.Stderr = stderr cmd.Args[0] = "godoc" @@ -205,8 +177,8 @@ func TestURL(t *testing.T) { // Basic integration test for godoc HTTP interface. func TestWeb(t *testing.T) { - bin, cleanup := buildGodoc(t) - defer cleanup() + bin := godocPath(t) + for _, x := range packagestest.All { t.Run(x.Name(), func(t *testing.T) { testWeb(t, x, bin, false) @@ -217,17 +189,19 @@ func TestWeb(t *testing.T) { // Basic integration test for godoc HTTP interface. func TestWebIndex(t *testing.T) { if testing.Short() { - t.Skip("skipping test in -short mode") + t.Skip("skipping slow test in -short mode") } - bin, cleanup := buildGodoc(t) - defer cleanup() + bin := godocPath(t) testWeb(t, packagestest.GOPATH, bin, true) } // Basic integration test for godoc HTTP interface. func testWeb(t *testing.T, x packagestest.Exporter, bin string, withIndex bool) { - if runtime.GOOS == "plan9" { - t.Skip("skipping on plan9; fails to start up quickly enough") + switch runtime.GOOS { + case "plan9": + t.Skip("skipping on plan9: fails to start up quickly enough") + case "android", "ios": + t.Skip("skipping on mobile: lacks GOROOT/api in test environment") } // Write a fake GOROOT/GOPATH with some third party packages. @@ -256,23 +230,39 @@ package a; import _ "godoc.test/repo2/a"; const Name = "repo1a"`, if withIndex { args = append(args, "-index", "-index_interval=-1s") } - cmd := exec.Command(bin, args...) + cmd := testenv.Command(t, bin, args...) cmd.Dir = e.Config.Dir cmd.Env = e.Config.Env - cmd.Stdout = os.Stderr - cmd.Stderr = os.Stderr + cmdOut := new(strings.Builder) + cmd.Stdout = cmdOut + cmd.Stderr = cmdOut cmd.Args[0] = "godoc" if err := cmd.Start(); err != nil { t.Fatalf("failed to start godoc: %s", err) } - defer killAndWait(cmd) + ctx, cancel := context.WithCancel(context.Background()) + go func() { + err := cmd.Wait() + t.Logf("%v: %v", cmd, err) + cancel() + }() + defer func() { + // Shut down the server cleanly if possible. + if runtime.GOOS == "windows" { + cmd.Process.Kill() // Windows doesn't support os.Interrupt. + } else { + cmd.Process.Signal(os.Interrupt) + } + <-ctx.Done() + t.Logf("server output:\n%s", cmdOut) + }() if withIndex { - waitForSearchReady(t, cmd, addr) + waitForSearchReady(t, ctx, cmd, addr) } else { - waitForServerReady(t, cmd, addr) - waitUntilScanComplete(t, addr) + waitForServerReady(t, ctx, cmd, addr) + waitUntilScanComplete(t, ctx, addr) } tests := []struct { @@ -454,22 +444,17 @@ func TestNoMainModule(t *testing.T) { if runtime.GOOS == "plan9" { t.Skip("skipping on plan9; for consistency with other tests that build godoc binary") } - bin, cleanup := buildGodoc(t) - defer cleanup() - tempDir, err := ioutil.TempDir("", "godoc-test-") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tempDir) + bin := godocPath(t) + tempDir := t.TempDir() // Run godoc in an empty directory with module mode explicitly on, // so that 'go env GOMOD' reports os.DevNull. - cmd := exec.Command(bin, "-url=/") + cmd := testenv.Command(t, bin, "-url=/") cmd.Dir = tempDir cmd.Env = append(os.Environ(), "GO111MODULE=on") var stderr bytes.Buffer cmd.Stderr = &stderr - err = cmd.Run() + err := cmd.Run() if err != nil { t.Fatalf("godoc command failed: %v\nstderr=%q", err, stderr.String()) }