Grouping subtests using test suites in test.xml (#4275)

**What type of PR is this?**
Feature

**What does this PR do? Why is it needed?**
This PR makes every test function a `<testsuite>` and every subtest a
`<testcase>`. This allow us to record the start time of every test
function, because only `<testsuite>` has a timestamp attribute. This
approach is also consistent with [testify
suite](https://pkg.go.dev/github.com/stretchr/testify/suite), each of
which becomes a `<testsuite>`.

Note that in the test.xml, a test function is both a `<testsuite>` and a
`<testcase>`. It has to be a `<testcase>` because the test function
itself has its own duration that is independent of its subtests. Also,
it can have assertions outside all subtests, it can fail even all its
subtests succeed. If no subtest is used, every `<testsuite>` only has
one `<testcase>`, which is more verbose than before.

Consumers of test.xml that only read `<testcase>` are not affected by
this change. but those assuming only one `<testsuite>` will need to be
updated.

We also lose track of the duration of the whole package, but that can be
obtained from Bazel build events.

Author: linzhp

go/tools/bzltestutil/testdata/empty.xml

@@ -1,3 +1 @@
-<testsuites>
-	<testsuite errors="0" failures="0" skipped="0" tests="0" time="" name="pkg/testing"></testsuite>
-</testsuites>
+<testsuites></testsuites>

go/tools/bzltestutil/testdata/report.xml

@@ -1,10 +1,16 @@
 <testsuites>
-	<testsuite errors="0" failures="3" skipped="1" tests="7" time="0.030" name="pkg/testing">
+	<testsuite errors="0" failures="1" skipped="0" tests="1" time="0.000" name="pkg/testing.TestFail">
 		<testcase classname="testing" name="TestFail" time="0.000">
 			<failure message="Failed" type="">=== RUN   TestFail&#xA;--- FAIL: TestFail (0.00s)&#xA;    test_test.go:23: Not working&#xA;</failure>
 		</testcase>
+	</testsuite>
+	<testsuite errors="0" failures="0" skipped="0" tests="1" time="0.000" name="pkg/testing.TestPass">
 		<testcase classname="testing" name="TestPass" time="0.000"></testcase>
+	</testsuite>
+	<testsuite errors="0" failures="0" skipped="0" tests="1" time="0.000" name="pkg/testing.TestPassLog">
 		<testcase classname="testing" name="TestPassLog" time="0.000"></testcase>
+	</testsuite>
+	<testsuite errors="0" failures="2" skipped="1" tests="4" time="0.020" name="pkg/testing.TestSubtests">
 		<testcase classname="testing" name="TestSubtests" time="0.020">
 			<failure message="Failed" type="">=== RUN   TestSubtests&#xA;--- FAIL: TestSubtests (0.02s)&#xA;</failure>
 		</testcase>

go/tools/bzltestutil/testdata/timeout.xml

@@ -1,5 +1,5 @@
 <testsuites>
-	<testsuite errors="2" failures="0" skipped="0" tests="5" time="8.555" name="pkg/testing" timestamp="2025-02-07T17:15:47.557Z">
+	<testsuite errors="2" failures="0" skipped="0" tests="5" time="8.000" name="pkg/testing.TestReport" timestamp="2025-02-07T17:15:48.107Z">
 		<testcase classname="testing" name="TestReport" time="8.000">
 			<error message="Interrupted" type="">=== RUN   TestReport&#xA;&#x9;&#x9;TestReport (8s)&#xA;</error>
 		</testcase>

go/tools/bzltestutil/xml.go

@@ -72,6 +72,7 @@ type testCase struct {
 	state    string
 	output   strings.Builder
 	duration *float64
+	timestamp *time.Time
 }
 
 const (
@@ -81,10 +82,6 @@ const (
 // json2xml converts test2json's output into an xml output readable by Bazel.
 // http://windyroad.com.au/dl/Open%20Source/JUnit.xsd
 func json2xml(r io.Reader, pkgName string) ([]byte, error) {
-	var (
-		pkgDuration  *float64
-		pkgTimestamp *time.Time
-	)
 	testcases := make(map[string]*testCase)
 	testCaseByName := func(name string) *testCase {
 		if name == "" {
@@ -105,24 +102,16 @@ func json2xml(r io.Reader, pkgName string) ([]byte, error) {
 		} else if err != nil {
 			return nil, fmt.Errorf("error decoding test2json output: %s", err)
 		}
-		if pkgTimestamp == nil || (e.Time != nil && e.Time.Unix() < pkgTimestamp.Unix()) {
-			pkgTimestamp = e.Time
-		}
 		switch s := e.Action; s {
 		case "run":
 			if c := testCaseByName(e.Test); c != nil {
 				c.state = s
+				c.timestamp = e.Time
 			}
 		case "output":
 			trimmedOutput := strings.TrimSpace(e.Output)
 			if strings.HasPrefix(trimmedOutput, timeoutPanicPrefix) {
 				inTimeoutSection = true
-				// the final "fail" action with "Elapsed" may not appear. If not, using the timeout setting as
-				// pkgDuration; if it appears, it will override pkgDuration.
-				if duration, err := time.ParseDuration(strings.TrimSpace(trimmedOutput[len(timeoutPanicPrefix):])); err == nil {
-					seconds := duration.Seconds()
-					pkgDuration = &seconds
-				}
 				continue
 			}
 			if inTimeoutSection && strings.HasPrefix(trimmedOutput, "running tests:") {
@@ -159,39 +148,48 @@ func json2xml(r io.Reader, pkgName string) ([]byte, error) {
 			if c := testCaseByName(e.Test); c != nil {
 				c.state = s
 				c.duration = e.Elapsed
-			} else {
-				pkgDuration = e.Elapsed
 			}
 		case "pass":
 			if c := testCaseByName(e.Test); c != nil {
 				c.duration = e.Elapsed
 				c.state = s
-			} else {
-				pkgDuration = e.Elapsed
 			}
 		}
 	}
 
-	return xml.MarshalIndent(toXML(pkgName, pkgDuration, pkgTimestamp, testcases), "", "\t")
+	return xml.MarshalIndent(toXML(pkgName, testcases), "", "\t")
 }
 
-func toXML(pkgName string, pkgDuration *float64, pkgTimestamp *time.Time, testcases map[string]*testCase) *xmlTestSuites {
+func toXML(pkgName string, testcases map[string]*testCase) *xmlTestSuites {
 	cases := make([]string, 0, len(testcases))
 	for k := range testcases {
 		cases = append(cases, k)
 	}
 	sort.Strings(cases)
-	suite := xmlTestSuite{
-		Name: pkgName,
-	}
-	if pkgDuration != nil {
-		suite.Time = fmt.Sprintf("%.3f", *pkgDuration)
-	}
-	if pkgTimestamp != nil {
-		suite.Timestamp = pkgTimestamp.Format("2006-01-02T15:04:05.000Z")
-	}
+
+	suiteByName := make(map[string]*xmlTestSuite)
+	var suiteNames []string
+
 	for _, name := range cases {
+		suiteName := strings.SplitN(name, "/", 2)[0]
+		var suite *xmlTestSuite
+		suite, ok := suiteByName[suiteName]
+		if !ok {
+			suite = &xmlTestSuite{
+				Name: pkgName + "." + suiteName,
+			}
+			suiteByName[suiteName] = suite
+			suiteNames = append(suiteNames, suiteName)
+		}
 		c := testcases[name]
+		if name == suiteName {
+			if c.duration != nil {
+				suite.Time = fmt.Sprintf("%.3f", *c.duration)
+			}
+			if c.timestamp != nil {
+				suite.Timestamp = c.timestamp.Format("2006-01-02T15:04:05.000Z")
+			}
+		}
 		suite.Tests++
 		newCase := xmlTestCase{
 			Name:      name,
@@ -230,5 +228,10 @@ func toXML(pkgName string, pkgDuration *float64, pkgTimestamp *time.Time, testca
 		}
 		suite.TestCases = append(suite.TestCases, newCase)
 	}
-	return &xmlTestSuites{Suites: []xmlTestSuite{suite}}
+	var suites xmlTestSuites
+	// because test cases are sorted by name, the suite names are also sorted.
+	for _, name := range suiteNames {
+		suites.Suites = append(suites.Suites, *suiteByName[name])
+	}
+	return &suites
 }

tests/core/go_test/xmlreport_test.go

@@ -104,28 +104,65 @@ func Test(t *testing.T) {
 			args: []string{"test", "//:xml_test"},
 			expected: xmlTestSuites{
 				XMLName: xml.Name{Local: "testsuites"},
-				Suites: []xmlTestSuite{{
-					XMLName:  xml.Name{Local: "testsuite"},
-					Name:     "github.com/bazelbuild/rules_go/tests/core/go_test/xml_test",
-					Errors:   0,
-					Failures: 3,
-					Tests:    3,
-				}},
+				Suites: []xmlTestSuite{
+					{
+						XMLName:  xml.Name{Local: "testsuite"},
+						Name:     "github.com/bazelbuild/rules_go/tests/core/go_test/xml_test.TestFail",
+						Errors:   0,
+						Failures: 1,
+						Skipped:  0,
+						Tests:    1,
+					},
+					{
+						XMLName:  xml.Name{Local: "testsuite"},
+						Name:     "github.com/bazelbuild/rules_go/tests/core/go_test/xml_test.TestSubtests",
+						Errors:   0,
+						Failures: 2,
+						Skipped:  0,
+						Tests:    2,
+					},
+				},
 			},
 		},
 		{
 			name: "verbose",
 			args: []string{"test", "--test_env=GO_TEST_WRAP_TESTV=1", "//:xml_test"},
 			expected: xmlTestSuites{
 				XMLName: xml.Name{Local: "testsuites"},
-				Suites: []xmlTestSuite{{
-					XMLName:  xml.Name{Local: "testsuite"},
-					Name:     "github.com/bazelbuild/rules_go/tests/core/go_test/xml_test",
-					Errors:   0,
-					Failures: 3,
-					Skipped:  1,
-					Tests:    7,
-				}},
+				Suites: []xmlTestSuite{
+					{
+						XMLName:  xml.Name{Local: "testsuite"},
+						Name:     "github.com/bazelbuild/rules_go/tests/core/go_test/xml_test.TestFail",
+						Errors:   0,
+						Failures: 1,
+						Skipped:  0,
+						Tests:    1,
+					},
+					{
+						XMLName:  xml.Name{Local: "testsuite"},
+						Name:     "github.com/bazelbuild/rules_go/tests/core/go_test/xml_test.TestPass",
+						Errors:   0,
+						Failures: 0,
+						Skipped:  0,
+						Tests:    1,
+					},
+					{
+						XMLName:  xml.Name{Local: "testsuite"},
+						Name:     "github.com/bazelbuild/rules_go/tests/core/go_test/xml_test.TestPassLog",
+						Errors:   0,
+						Failures: 0,
+						Skipped:  0,
+						Tests:    1,
+					},
+					{
+						XMLName:  xml.Name{Local: "testsuite"},
+						Name:     "github.com/bazelbuild/rules_go/tests/core/go_test/xml_test.TestSubtests",
+						Errors:   0,
+						Failures: 2,
+						Skipped:  1,
+						Tests:    4,
+					},
+				},
 			},
 		},
 	}