Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting test.timeout according to TEST_TIMEOUT #3920

Merged
merged 3 commits into from
Apr 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion go/tools/builders/generate_test_main.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,13 @@ import (
"log"
"os"
"os/exec"
"os/signal"
{{if .TestMain}}
"reflect"
{{end}}
"strconv"
"strings"
"syscall"
"testing"
"testing/internal/testdeps"

Expand Down Expand Up @@ -235,7 +237,19 @@ func main() {
}
}
{{end}}
bzltestutil.RegisterTimeoutHandler()

testTimeout := os.Getenv("TEST_TIMEOUT")
if testTimeout != "" {
flag.Lookup("test.timeout").Value.Set(testTimeout+"s")
// If Bazel sends a SIGTERM because the test timed out, it sends it to all child processes. Because
// we set -test.timeout according to the TEST_TIMEOUT, we need to ignore the signal so the test has
// time to properly produce the output (e.g. stack trace). It will be killed by Bazel after the grace
// period (15s) expires.
// If TEST_TIMEOUT is not set (e.g., when the test binary is run by Delve for debugging), we don't
// ignore SIGTERM so it can be properly terminated.
signal.Ignore(syscall.SIGTERM)
linzhp marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sluongng @fmeum Maybe we should do this instead here?

	c := make(chan os.Signal, 1)
	signal.Notify(c, syscall.SIGTERM)
	go func() {
		<-c
	}()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the intent of this being resettable by signal.Reset? I like that.

}

{{if not .TestMain}}
res := m.Run()
{{else}}
Expand Down
1 change: 0 additions & 1 deletion go/tools/bzltestutil/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ go_tool_library(
srcs = [
"lcov.go",
"test2json.go",
"timeout.go",
"wrap.go",
"xml.go",
],
Expand Down
39 changes: 0 additions & 39 deletions go/tools/bzltestutil/timeout.go

This file was deleted.

9 changes: 5 additions & 4 deletions go/tools/bzltestutil/wrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,11 @@ func Wrap(pkg string) error {
exePath = filepath.Join(chdir.TestExecDir, exePath)
}

// If Bazel sends a SIGTERM because the test timed out, it sends it to all child processes. As
// a result, the child process will print stack traces of all Go routines and we want the
// wrapper to be around to capute and forward this output. Thus, we need to ignore the signal
// and will be killed by Bazel after the grace period instead.
// If Bazel sends a SIGTERM because the test timed out, it sends it to all child processes. However,
// we want the wrapper to be around to capute and forward the test output when this happens. Thus,
// we need to ignore the signal. This wrapper will natually ends after the Go test ends, either by
// SIGTERM or the time set by -test.timeout expires. If that doesn't happen, the test and this warpper
// will be killed by Bazel after the grace period (15s) expires.
signal.Ignore(syscall.SIGTERM)

cmd := exec.Command(exePath, args...)
Expand Down
27 changes: 23 additions & 4 deletions tests/core/go_test/timeout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,19 @@ func TestTimeout(t *testing.T) {
t.Skip("stack traces on timeouts are not yet supported on Windows")
}

if err := bazel_testing.RunBazel("test", "//:timeout_test", "--test_timeout=3"); err == nil {
var stderr string
if err := bazel_testing.RunBazel("test", "//:timeout_test", "--test_timeout=3", "--test_arg=-test.v"); err == nil {
t.Fatal("expected bazel test to fail")
} else if exitErr, ok := err.(*bazel_testing.StderrExitError); !ok || exitErr.Err.ExitCode() != 3 {
t.Fatalf("expected bazel test to fail with exit code 3", err)
} else {
stderr = string(exitErr.Err.Stderr)
}

if !strings.Contains(stderr, "TIMEOUT: //:timeout_test") {
t.Errorf("expect Bazel to report the test timed out: \n%s", stderr)
}

p, err := bazel_testing.BazelOutput("info", "bazel-testlogs")
if err != nil {
t.Fatalf("could not find testlogs root: %s", err)
Expand All @@ -57,10 +65,21 @@ func TestTimeout(t *testing.T) {
}

testLog := string(b)
if !strings.Contains(testLog, "Received SIGTERM, printing stack traces of all goroutines:") {
t.Fatalf("test log does not contain expected header:\n%s", testLog)
if !strings.Contains(testLog, "panic: test timed out after 3s") {
t.Errorf("test log does not contain expected header:\n%s", testLog)
}
if !strings.Contains(testLog, "timeout_test.neverTerminates(") {
t.Fatalf("test log does not contain expected stack trace:\n%s", testLog)
t.Errorf("test log does not contain expected stack trace:\n%s", testLog)
}

path = filepath.Join(strings.TrimSpace(string(p)), "timeout_test/test.xml")
b, err = os.ReadFile(path)
if err != nil {
t.Fatalf("could not read test XML: %s", err)
}

testXML := string(b)
if !strings.Contains(testXML, `<testcase classname="timeout_test" name="TestFoo"`) {
t.Errorf("test XML does not contain expected element:\n%s", testXML)
}
}