Make VCR correctly handle compound tests (#13941)

Author: trodge

.ci/magician/cmd/templates/vcr/record_replay.tmpl

@@ -11,7 +11,7 @@
 {{color "red" "Tests failed when rerunning REPLAYING mode:"}}
 {{range .ReplayingAfterRecordingResult.FailedTests -}}
 `{{.}}` {{/* remove trailing whitespace */ -}}
-  [[Error message](https://storage.cloud.google.com/{{$.LogBucket}}/{{$.Version}}/refs/heads/{{$.Head}}/artifacts/{{$.BuildID}}/build-log/replaying_build_after_recording/{{.}}_replaying_test.log)] {{/* remove trailing whitespace */ -}}
+  [[Error message](https://storage.cloud.google.com/{{$.LogBucket}}/{{$.Version}}/refs/heads/{{$.Head}}/artifacts/{{$.BuildID}}/build-log/replaying_build_after_recording/{{compoundTest .}}_replaying_test.log)] {{/* remove trailing whitespace */ -}}
   [[Debug log](https://storage.cloud.google.com/{{$.LogBucket}}/{{$.Version}}/refs/heads/{{$.Head}}/artifacts/{{$.BuildID}}/replaying_after_recording/{{.}}.log)]
 {{/* remove trailing whitespace */ -}}
 {{end}}
@@ -30,7 +30,7 @@ Please fix these to complete your PR. If you believe these test failures to be i
 {{color "red" "Tests failed during RECORDING mode:"}}
 {{range .RecordingResult.FailedTests -}}
 `{{.}}` {{/* remove trailing whitespace */ -}}
-  [[Error message](https://storage.cloud.google.com/{{$.LogBucket}}/{{$.Version}}/refs/heads/{{$.Head}}/artifacts/{{$.BuildID}}/build-log/recording_build/{{.}}_recording_test.log)] {{/* remove trailing whitespace */ -}}
+  [[Error message](https://storage.cloud.google.com/{{$.LogBucket}}/{{$.Version}}/refs/heads/{{$.Head}}/artifacts/{{$.BuildID}}/build-log/recording_build/{{compoundTest .}}_recording_test.log)] {{/* remove trailing whitespace */ -}}
   [[Debug log](https://storage.cloud.google.com/{{$.LogBucket}}/{{$.Version}}/refs/heads/{{$.Head}}/artifacts/{{$.BuildID}}/recording/{{.}}.log)]
 {{/* remove trailing whitespace */ -}}
 {{end}}

.ci/magician/cmd/test_terraform_vcr.go

@@ -235,12 +235,13 @@ func execTestTerraformVCR(prNumber, mmCommitSha, buildID, projectID, buildStep,
 	}
 
 	notRunBeta, notRunGa := notRunTests(tpgRepo.UnifiedZeroDiff, tpgbRepo.UnifiedZeroDiff, replayingResult)
+
 	postReplayData := postReplay{
 		RunFullVCR:       runFullVCR,
 		AffectedServices: sort.StringSlice(servicesArr),
 		NotRunBetaTests:  notRunBeta,
 		NotRunGATests:    notRunGa,
-		ReplayingResult:  replayingResult,
+		ReplayingResult:  subtestResult(replayingResult),
 		ReplayingErr:     replayingErr,
 		LogBucket:        "ci-vcr-logs",
 		Version:          provider.Beta.String(),
@@ -318,8 +319,8 @@ func execTestTerraformVCR(prNumber, mmCommitSha, buildID, projectID, buildStep,
 		allRecordingPassed := len(recordingResult.FailedTests) == 0 && !hasTerminatedTests && recordingErr == nil
 
 		recordReplayData := recordReplay{
-			RecordingResult:               recordingResult,
-			ReplayingAfterRecordingResult: replayingAfterRecordingResult,
+			RecordingResult:               subtestResult(recordingResult),
+			ReplayingAfterRecordingResult: subtestResult(replayingAfterRecordingResult),
 			RecordingErr:                  recordingErr,
 			HasTerminatedTests:            hasTerminatedTests,
 			AllRecordingPassed:            allRecordingPassed,
@@ -386,6 +387,43 @@ func notRunTests(gaDiff, betaDiff string, result vcr.Result) ([]string, []string
 	return notRunBeta, notRunGa
 }
 
+func subtestResult(original vcr.Result) vcr.Result {
+	return vcr.Result{
+		PassedTests:  excludeCompoundTests(original.PassedTests, original.PassedSubtests),
+		FailedTests:  excludeCompoundTests(original.FailedTests, original.FailedSubtests),
+		SkippedTests: excludeCompoundTests(original.SkippedTests, original.SkippedSubtests),
+		Panics:       original.Panics,
+	}
+}
+
+// Returns the name of the compound test that the given subtest belongs to.
+func compoundTest(subtest string) string {
+	parts := strings.Split(subtest, "__")
+	if len(parts) != 2 {
+		return subtest
+	}
+	return parts[0]
+}
+
+// Returns subtests and tests that are not compound tests.
+func excludeCompoundTests(allTests, subtests []string) []string {
+	res := make([]string, 0, len(allTests)+len(subtests))
+	compoundTests := make(map[string]struct{}, len(subtests))
+	for _, subtest := range subtests {
+		if compound := compoundTest(subtest); compound != subtest {
+			compoundTests[compound] = struct{}{}
+			res = append(res, subtest)
+		}
+	}
+	for _, test := range allTests {
+		if _, ok := compoundTests[test]; !ok {
+			res = append(res, test)
+		}
+	}
+	sort.Strings(res)
+	return res
+}
+
 func modifiedPackages(changedFiles []string, version provider.Version) (map[string]struct{}, bool) {
 	var goFiles []string
 	for _, line := range changedFiles {
@@ -468,9 +506,10 @@ func init() {
 
 func formatComment(fileName string, tmplText string, data any) (string, error) {
 	funcs := template.FuncMap{
-		"join":  strings.Join,
-		"add":   func(i, j int) int { return i + j },
-		"color": color,
+		"join":         strings.Join,
+		"add":          func(i, j int) int { return i + j },
+		"color":        color,
+		"compoundTest": compoundTest,
 	}
 	tmpl, err := template.New(fileName).Funcs(funcs).Parse(tmplText)
 	if err != nil {

.ci/magician/vcr/tester.go

@@ -13,10 +13,13 @@ import (
 )
 
 type Result struct {
-	PassedTests  []string
-	SkippedTests []string
-	FailedTests  []string
-	Panics       []string
+	PassedTests     []string
+	SkippedTests    []string
+	FailedTests     []string
+	PassedSubtests  []string
+	SkippedSubtests []string
+	FailedSubtests  []string
+	Panics          []string
 }
 
 type Mode int
@@ -66,6 +69,8 @@ const replayingTimeout = "240m"
 
 var testResultsExpression = regexp.MustCompile(`(?m:^--- (PASS|FAIL|SKIP): (TestAcc\w+))`)
 
+var subtestResultsExpression = regexp.MustCompile(`(?m:^    --- (PASS|FAIL|SKIP): (TestAcc\w+)/(\w+))`)
+
 var testPanicExpression = regexp.MustCompile(`^panic: .*`)
 
 var safeToLog = map[string]bool{
@@ -603,19 +608,39 @@ func collectResult(output string) Result {
 		}
 		resultSets[submatches[1]][submatches[2]] = struct{}{}
 	}
+	matches = subtestResultsExpression.FindAllStringSubmatch(output, -1)
+	subtestResultSets := make(map[string]map[string]struct{}, 4)
+	for _, submatches := range matches {
+		if len(submatches) != 4 {
+			fmt.Printf("Warning: unexpected regex match found in test output: %v", submatches)
+			continue
+		}
+		if _, ok := subtestResultSets[submatches[1]]; !ok {
+			subtestResultSets[submatches[1]] = make(map[string]struct{})
+		}
+		subtestResultSets[submatches[1]][fmt.Sprintf("%s__%s", submatches[2], submatches[3])] = struct{}{}
+	}
 	results := make(map[string][]string, 4)
 	results["PANIC"] = testPanicExpression.FindAllString(output, -1)
 	sort.Strings(results["PANIC"])
+	subtestResults := make(map[string][]string, 3)
 	for _, kind := range []string{"FAIL", "PASS", "SKIP"} {
 		for test := range resultSets[kind] {
 			results[kind] = append(results[kind], test)
 		}
 		sort.Strings(results[kind])
+		for subtest := range subtestResultSets[kind] {
+			subtestResults[kind] = append(subtestResults[kind], subtest)
+		}
+		sort.Strings(subtestResults[kind])
 	}
 	return Result{
-		FailedTests:  results["FAIL"],
-		PassedTests:  results["PASS"],
-		SkippedTests: results["SKIP"],
-		Panics:       results["PANIC"],
+		FailedTests:     results["FAIL"],
+		PassedTests:     results["PASS"],
+		SkippedTests:    results["SKIP"],
+		FailedSubtests:  subtestResults["FAIL"],
+		PassedSubtests:  subtestResults["PASS"],
+		SkippedSubtests: subtestResults["SKIP"],
+		Panics:          results["PANIC"],
 	}
 }

.ci/magician/vcr/tester_test.go

@@ -0,0 +1,58 @@
+package vcr
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestCollectResults(t *testing.T) {
+	for _, test := range []struct {
+		name     string
+		output   string
+		expected Result
+	}{
+		{
+			name: "no compound tests",
+			output: `--- FAIL: TestAccServiceOneResourceOne (100.00s)
+--- PASS: TestAccServiceOneResourceTwo (100.00s)
+--- PASS: TestAccServiceTwoResourceOne (100.00s)
+--- PASS: TestAccServiceTwoResourceTwo (100.00s)
+`,
+			expected: Result{
+				PassedTests: []string{"TestAccServiceOneResourceTwo", "TestAccServiceTwoResourceOne", "TestAccServiceTwoResourceTwo"},
+				FailedTests: []string{"TestAccServiceOneResourceOne"},
+			},
+		},
+		{
+			name: "compound tests",
+			output: `--- FAIL: TestAccServiceOneResourceOne (100.00s)
+--- FAIL: TestAccServiceOneResourceTwo (100.00s)
+    --- PASS: TestAccServiceOneResourceTwo/test_one (100.00s)
+    --- FAIL: TestAccServiceOneResourceTwo/test_two (100.00s)
+--- PASS: TestAccServiceTwoResourceOne (100.00s)
+    --- PASS: TestAccServiceTwoResourceOne/test_one (100.00s)
+    --- PASS: TestAccServiceTwoResourceOne/test_two (100.00s)
+--- PASS: TestAccServiceTwoResourceTwo (100.00s)
+`,
+			expected: Result{
+				PassedTests: []string{
+					"TestAccServiceTwoResourceOne",
+					"TestAccServiceTwoResourceTwo",
+				},
+				FailedTests: []string{"TestAccServiceOneResourceOne", "TestAccServiceOneResourceTwo"},
+				PassedSubtests: []string{
+					"TestAccServiceOneResourceTwo__test_one",
+					"TestAccServiceTwoResourceOne__test_one",
+					"TestAccServiceTwoResourceOne__test_two",
+				},
+				FailedSubtests: []string{"TestAccServiceOneResourceTwo__test_two"},
+			},
+		},
+	} {
+		if diff := cmp.Diff(test.expected, collectResult(test.output)); diff != "" {
+			t.Errorf("collectResult(%q) got unexpected diff (-want +got):\n%s", test.output, diff)
+		}
+	}
+
+}