bazel-contrib / bazel-contrib/rules_go

Flip GO_TEST_WRAP_TESTV default

Open
#3,835 6 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
1.5k
Forks
760
Avg merge
1d 11h
Merged PRs (30d)
12

Description

In https://github.com/bazelbuild/rules_go/issues/2364, it was established that `GO_TEST_WRAP_TESTV` will be opt-in instead of opt-out. The main reason was `--test_output=errors` output will contain more verbose outputs of successful test cases being mixed among failed test cases.

For example

```bash
> cat main_test.go
package main

import "testing"

func TestHelloWorld(t *testing.T) {
}

func TestHelloWorld2(t *testing.T) {
}

func TestHelloWorld3(t *testing.T) {
}

func TestHelloWorld4(t *testing.T) {
}

func TestHelloWorld5(t *testing.T) {
t.Fatal("blah")
}

> bazel 2>/dev/null test :all --test_env=GO_TEST_WRAP_TESTV=1
==================== Test output for //:a_test:
=== RUN TestHelloWorld
--- PASS: TestHelloWorld (0.00s)
=== RUN TestHelloWorld2
--- PASS: TestHelloWorld2 (0.00s)
=== RUN TestHelloWorld3
--- PASS: TestHelloWorld3 (0.00s)
=== RUN TestHelloWorld4
--- PASS: TestHelloWorld4 (0.00s)
=== RUN TestHelloWorld5
main_test.go:18: blah
--- FAIL: TestHelloWorld5 (0.00s)
FAIL
================================================================================
//:a_test FAILED in 0.0s
/private/var/tmp/_bazel_sluongng/f16739de4a8438c42e8aba41e8e74b32/execroot/_main/bazel-out/darwin_arm64-fastbuild/testlogs/a_test/test.log

Executed 1 out of 1 test: 1 fails locally.
exit 3

> bazel 2>/dev/null test :all --test_env=GO_TEST_WRAP_TESTV=0
==================== Test output for //:a_test:
--- FAIL: TestHelloWorld5 (0.00s)
main_test.go:18: blah
FAIL
================================================================================
//:a_test FAILED in 0.0s
/private/var/tmp/_bazel_sluongng/f16739de4a8438c42e8aba41e8e74b32/execroot/_main/bazel-out/darwin_arm64-fastbuild/testlogs/a_test/test.log

Executed 1 out of 1 test: 1 fails locally.
exit 3
```

However, I think we should flip this to opt-out instead of opt-in. The reasons are:

1. `--test_output=errors` is not the default in Bazel. To make it a default, the user would have to set it in a custom `.bazelrc` file, and in there, the opt-out could be set accordingly to achieve desired outputs.

2. In the happy case where all tests succeed, the `test.xml` file will be created by parsing the verbose output. Currently, because of the default non-verbose output, the test.xml file is relatively empty and useless. However, with verbose output, each test `func` would be converted to a `testcase` in the XML, decorated with execution time to aid test performance investigation. This is especially potent when used with BES service that could help analyze these XMLs overtime.

3. Big test targets could be sharded to reduce the amount of `func` included in each shard. Note that Bazel does cap the maximum shard count to 25 to help promote smaller targets/packages. With the addition of https://github.com/bazelbuild/bazel-gazelle/pull/1597, user would now have more ways to reduce their `go_test` target size

So I think a modest change like this

```diff
diff --git a/go/tools/bzltestutil/wrap.go b/go/tools/bzltestutil/wrap.go
index 511768c8..c3ccbf75 100644
--- a/go/tools/bzltestutil/wrap.go
+++ b/go/tools/bzltestutil/wrap.go
@@ -61,7 +61,7 @@ func shouldAddTestV() bool {
}
return wrap
}
- return false
+ return true
}

```

would help improve new users' experience while still allowing power users to optionally opt-out with `--test_env=GO_TEST_WRAP_TESTV=0`

Contributor guide

Open the contributing guide

Research direction

Start in go/tools/bzltestutil/wrap.go at shouldAddTestV and review how GO_TEST_WRAP_TESTV is handled. Verify that the default enables verbose test output while GO_TEST_WRAP_TESTV=0 still disables it, then run the relevant bzltestutil tests or test commands. Done means the default behavior is flipped without removing the opt-out.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
build-system, testing
Issue type
Feature
Difficulty
1/5
Estimated time
Under an hour
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.