From f808fd74404975d441a113129f03c2bfafca80f9 Mon Sep 17 00:00:00 2001 From: Daniel Wagner-Hall Date: Fri, 12 Jan 2024 15:02:48 +0000 Subject: [PATCH] go_test: Allow not registering a SIGTERM handler The rules_go SIGTERM handler causes tests which themselves test responding to SIGTERM to panic and fail. --- docs/go/core/rules.md | 3 +- go/private/rules/test.bzl | 9 ++++++ go/tools/builders/generate_test_main.go | 35 +++++++++++++-------- tests/core/go_test/BUILD.bazel | 10 ++++++ tests/core/go_test/sigterm_handler_test.go | 36 ++++++++++++++++++++++ 5 files changed, 80 insertions(+), 13 deletions(-) create mode 100644 tests/core/go_test/sigterm_handler_test.go diff --git a/docs/go/core/rules.md b/docs/go/core/rules.md index 3dd32e775b..0331f6c1b7 100644 --- a/docs/go/core/rules.md +++ b/docs/go/core/rules.md @@ -334,7 +334,7 @@ This declares a set of source files and related dependencies that can be embedde 
 go_test(name, cdeps, cgo, clinkopts, copts, cppopts, cxxopts, data, deps, embed, embedsrcs, env,
         env_inherit, gc_goopts, gc_linkopts, goarch, goos, gotags, importpath, linkmode, msan, pure,
-        race, rundir, srcs, static, x_defs)
+        race, register_timeout_handler, rundir, srcs, static, x_defs)
This builds a set of tests that can be run with `bazel test`.

@@ -396,6 +396,7 @@ This builds a set of tests that can be run with `bazel test`.

| msan | Controls whether code is instrumented for memory sanitization. May be one of on, off, or auto. Not available when cgo is disabled. In most cases, it's better to control this on the command line with --@io_bazel_rules_go//go/config:msan. See [mode attributes], specifically [msan]. | String | optional | "auto" | | pure | Controls whether cgo source code and dependencies are compiled and linked, similar to setting CGO_ENABLED. May be one of on, off, or auto. If auto, pure mode is enabled when no C/C++ toolchain is configured or when cross-compiling. It's usually better to control this on the command line with --@io_bazel_rules_go//go/config:pure. See [mode attributes], specifically [pure]. | String | optional | "auto" | | race | Controls whether code is instrumented for race detection. May be one of on, off, or auto. Not available when cgo is disabled. In most cases, it's better to control this on the command line with --@io_bazel_rules_go//go/config:race. See [mode attributes], specifically [race]. | String | optional | "auto" | +| register_timeout_handler | Whether to register a SIGTERM handler to give improved output on test timeouts.

Setting this to False may be useful if your test itself also handles signals. | Boolean | optional | True | | rundir | A directory to cd to before the test is run. This should be a path relative to the root directory of the repository in which the test is defined, which can be the main or an external repository.

The default behaviour is to change to the relative path corresponding to the test's package, which replicates the normal behaviour of go test so it is easy to write compatible tests.

Setting it to . makes the test behave the normal way for a bazel test, except that the working directory is always that of the test's repository, which is not necessarily the main repository.

Note: If runfile symlinks are disabled (such as on Windows by default), the test will run in the working directory set by Bazel, which is the subdirectory of the runfiles directory corresponding to the main repository. | String | optional | "" | | srcs | The list of Go source files that are compiled to create the package. Only .go and .s files are permitted, unless the cgo attribute is set, in which case, .c .cc .cpp .cxx .h .hh .hpp .hxx .inc .m .mm files are also permitted. Files may be filtered at build time using Go [build constraints]. | List of labels | optional | [] | | static | Controls whether a binary is statically linked. May be one of on, off, or auto. Not available on all platforms or in all modes. It's usually better to control this on the command line with --@io_bazel_rules_go//go/config:static. See [mode attributes], specifically [static]. | String | optional | "auto" | diff --git a/go/private/rules/test.bzl b/go/private/rules/test.bzl index 1283776a95..abf5925010 100644 --- a/go/private/rules/test.bzl +++ b/go/private/rules/test.bzl @@ -112,6 +112,9 @@ def _go_test_impl(ctx): "l_test=" + external_source.library.importpath, ) arguments.add("-pkgname", internal_source.library.importpath) + + # Boolean Go flags require =, so we can't just use the attr as an arg value, we need to convert it ourselves. + arguments.add("-register_timeout_handler={}".format("true" if ctx.attr.register_timeout_handler else "false")) arguments.add_all(go_srcs, before_each = "-src", format_each = "l=%s") ctx.actions.run( inputs = go_srcs, @@ -415,6 +418,12 @@ _go_test_kwargs = { See [Cross compilation] for more information. """, ), + "register_timeout_handler": attr.bool( + default = True, + doc = """Whether to register a SIGTERM handler to give improved output on test timeouts. + + Setting this to False may be useful if your test itself also handles signals.""", + ), "_go_context_data": attr.label(default = "//:go_context_data", cfg = go_transition), "_testmain_additional_deps": attr.label_list( providers = [GoLibrary], diff --git a/go/tools/builders/generate_test_main.go b/go/tools/builders/generate_test_main.go index c061d5e993..a6279e4889 100644 --- a/go/tools/builders/generate_test_main.go +++ b/go/tools/builders/generate_test_main.go @@ -63,15 +63,16 @@ type Example struct { // Cases holds template data. type Cases struct { - Imports []*Import - Tests []TestCase - Benchmarks []TestCase - FuzzTargets []TestCase - Examples []Example - TestMain string - CoverMode string - CoverFormat string - Pkgname string + Imports []*Import + Tests []TestCase + Benchmarks []TestCase + FuzzTargets []TestCase + Examples []Example + TestMain string + CoverMode string + CoverFormat string + Pkgname string + RegisterTimeoutHandler bool } // Version returns whether v is a supported Go version (like "go1.18"). @@ -235,7 +236,9 @@ func main() { } } {{end}} + {{if .RegisterTimeoutHandler}} bzltestutil.RegisterTimeoutHandler() + {{end}} {{if not .TestMain}} res := m.Run() {{else}} @@ -261,6 +264,7 @@ func genTestMain(args []string) error { coverMode := flags.String("cover_mode", "", "the coverage mode to use") coverFormat := flags.String("cover_format", "", "the coverage report type to generate (go_cover or lcov)") pkgname := flags.String("pkgname", "", "package name of test") + registerTimeoutHandler := flags.Bool("register_timeout_handler", true, "whether to register a SIGTERM handler for improved timeout reporting") flags.Var(&imports, "import", "Packages to import") flags.Var(&sources, "src", "Sources to process for tests") if err := flags.Parse(args); err != nil { @@ -269,6 +273,12 @@ func genTestMain(args []string) error { if err := goenv.checkFlags(); err != nil { return err } + // Boolean flags must be specified using `--foo=bar`, not `--foo bar`. + // Verify that there are no unexpected positional arguments, which may be the case if e.g. someone naively called + // args.add("-register_timeout_handler", ctx.attr.register_timeout_handler) + if len(flag.Args()) > 0 { + return fmt.Errorf("unexpected positional args: %v", flag.Args()) + } // Process import args importMap := map[string]*Import{} for _, imp := range imports { @@ -309,9 +319,10 @@ func genTestMain(args []string) error { } cases := Cases{ - CoverFormat: *coverFormat, - CoverMode: *coverMode, - Pkgname: *pkgname, + CoverFormat: *coverFormat, + CoverMode: *coverMode, + Pkgname: *pkgname, + RegisterTimeoutHandler: *registerTimeoutHandler, } testFileSet := token.NewFileSet() diff --git a/tests/core/go_test/BUILD.bazel b/tests/core/go_test/BUILD.bazel index e44564cca9..3adc05e242 100644 --- a/tests/core/go_test/BUILD.bazel +++ b/tests/core/go_test/BUILD.bazel @@ -250,6 +250,16 @@ go_test( shard_count = 2, ) +go_test( + name = "sigterm_handler_test", + srcs = ["sigterm_handler_test.go"], + register_timeout_handler = False, + target_compatible_with = select({ + "@platforms//os:windows": ["@platforms//:incompatible"], + "//conditions:default": [], + }), +) + go_bazel_test( name = "env_inherit_test", srcs = ["env_inherit_test.go"], diff --git a/tests/core/go_test/sigterm_handler_test.go b/tests/core/go_test/sigterm_handler_test.go new file mode 100644 index 0000000000..fd5bc875ea --- /dev/null +++ b/tests/core/go_test/sigterm_handler_test.go @@ -0,0 +1,36 @@ +package sigterm_handler_test + +import ( + "os" + "os/signal" + "sync" + "syscall" + "testing" +) + +func TestRegisterSignalHandler(t *testing.T) { + called := false + var wg sync.WaitGroup + + wg.Add(1) + + c := make(chan os.Signal, 1) + signal.Notify(c, syscall.SIGTERM) + + go func() { + switch <-c { + case syscall.SIGTERM: + called = true + wg.Done() + } + }() + + if err := syscall.Kill(os.Getpid(), syscall.SIGTERM); err != nil { + t.Fatalf("Failed to send SIGTERM: %v", err) + } + wg.Wait() + + if !called { + t.Fatal("Our handler has not run") + } +}