testrunner: don't mask a parent test that fails on its own (#5698)

## Summary

The integration test runner masks failures listed in
`known_failures.txt`. A known-failure rule names a specific subtest
(e.g. `TestAccept/ssh/connection`), but a parent test also fails
whenever one of its subtests fails, so the runner additionally allowed
the parent to fail. It did this by matching a rule against the parent
*bidirectionally*: a rule whose pattern was a descendant of the failing
test also matched.

That allowance was unconditional. When the top-level `TestAccept` failed
on its own — before any subtest ran — its failure still matched any
`TestAccept/...` rule and was silently swallowed. The recent
`RequireRuff` setup check (#5662) does exactly this on the integration
runners (ruff isn't installed there), so `TestAccept` has been failing
on every environment while the nightly reports green.

This moves the parent-cascade handling out of rule matching and into
failure analysis:

- `ConfigRule.matches` reverts to plain forward matching — a rule
matches a test or its subtree, nothing about parents.
- `checkFailures` allows a failure when a rule matches **or** a subtest
of it also failed. Go reports a parent's failure only after its
subtests, so the failing subtest is already recorded when the parent is
evaluated; no second pass is needed.

Net effect: a parent failing because of a (known or unknown) subtest is
still not double-counted, but a test that fails on its own with no
failing subtest now surfaces as unexpected.

This only makes the failure visible. Installing ruff on the integration
runners is handled separately.

## Tests

- `TestCheckFailures` exercises `checkFailures` end-to-end:
parent-with-failing-subtest (allowed), parent-alone (the ruff case,
surfaced), unlisted failure (surfaced), and fail-then-pass-on-retry
(allowed).
- `TestConfigRuleMatches` reverse-match cases now assert that a rule
does not match a parent of the listed test.

This pull request and its description were written by Isaac.

Author: pietern

tools/testrunner/main.go

@@ -6,6 +6,11 @@ Download https://github.com/databricks/cli/blob/ciconfig/known_failures.txt in p
 If download was successful by the time test runner finishes and test runner finishes with non-zero code,
 analyze failures in the test output identified by --jsonfile option. If all failures are expected (listed in known_failures.txt)
 then the failure is masked, the process exits with 0.
+
+A parent test fails whenever one of its subtests fails, so a rule that lists a
+subtest also allows the parent to fail. That allowance only applies when a
+subtest actually failed; if the parent failed on its own (e.g. in setup, before
+any subtest ran), its failure is reported as unexpected.
 */
 package main
 
@@ -158,6 +163,7 @@ func checkFailures(config *Config, jsonFile string, originalExitCode int) int {
 	defer func() { _ = file.Close() }()
 
 	scanner := bufio.NewScanner(file)
+	failed := map[string]bool{}
 	unexpectedFailures := map[string]bool{}
 
 	for scanner.Scan() {
@@ -177,21 +183,33 @@ func checkFailures(config *Config, jsonFile string, originalExitCode int) int {
 		}
 
 		result.Package, _ = strings.CutPrefix(result.Package, cutPrefix)
-
 		key := result.Package + " " + result.Test
 
-		if result.Action == "fail" {
+		switch result.Action {
+		case "fail":
+			// Go reports a parent's failure only after its subtests, so any
+			// failing subtest is already in `failed` by the time we get here.
+			failed[key] = true
 			matchedRule := config.matches(result.Package, result.Test)
-			if matchedRule != "" {
+			switch {
+			case matchedRule != "":
 				fmt.Printf("%s %s failure is allowed, matches rule %q\n", result.Package, result.Test, matchedRule)
-			} else {
+			case hasFailedSubtest(failed, key):
+				// A parent test fails when a subtest fails; the subtest is
+				// reported on its own, so the parent's failure is not counted.
+				fmt.Printf("%s %s failure is allowed, a subtest failed\n", result.Package, result.Test)
+			default:
 				fmt.Printf("%s %s failure is not allowed\n", result.Package, result.Test)
 				unexpectedFailures[key] = true
 			}
-		} else if result.Action == "pass" && unexpectedFailures[key] {
-			fmt.Printf("%s %s passed on retry\n", result.Package, result.Test)
-			// We run gotestsum with --rerun-fails, so we need to account for intermittent failures
-			delete(unexpectedFailures, key)
+		case "pass":
+			// We run gotestsum with --rerun-fails, so a test that fails and
+			// later passes is flaky, not a failure.
+			delete(failed, key)
+			if unexpectedFailures[key] {
+				fmt.Printf("%s %s passed on retry\n", result.Package, result.Test)
+				delete(unexpectedFailures, key)
+			}
 		}
 	}
 
@@ -207,6 +225,18 @@ func checkFailures(config *Config, jsonFile string, originalExitCode int) int {
 	return originalExitCode
 }
 
+// hasFailedSubtest reports whether any failing test is a strict subtest of key.
+// Keys are "<package> <test>", so a subtest's key is prefixed by key + "/".
+func hasFailedSubtest(failed map[string]bool, key string) bool {
+	prefix := key + "/"
+	for k := range failed {
+		if strings.HasPrefix(k, prefix) {
+			return true
+		}
+	}
+	return false
+}
+
 // CI Config Format
 //
 // The CI config is downloaded from the "ciconfig" branch of the repository.
@@ -328,9 +358,9 @@ func (r ConfigRule) matches(packageName, testName string) bool {
 
 	// Check test pattern
 	if r.TestPrefix {
-		return matchesPathPrefix(testName, r.TestPattern) || matchesPathPrefix(r.TestPattern, testName)
+		return matchesPathPrefix(testName, r.TestPattern)
 	}
-	return testName == r.TestPattern || matchesPathPrefix(r.TestPattern, testName)
+	return testName == r.TestPattern
 }
 
 // matchesPathPrefix returns true if s matches pattern or starts with pattern + "/"

tools/testrunner/main_test.go

@@ -1,6 +1,10 @@
 package main
 
 import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -44,17 +48,18 @@ func TestConfigRuleMatches(t *testing.T) {
 		{"* TestDeploy", "", "TestDeploy", true},
 		{"bundle *", "bundle", "", true},
 
-		// Subtest failure results in parent test failure as well. So we allow strict prefixes to fail as well
-		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic", "acceptance", "TestAccept", true},
+		// A rule does not match a parent of the listed test; the parent's
+		// cascading failure is handled separately (see TestCheckFailures).
+		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic", "acceptance", "TestAccept", false},
 		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic", "acceptance", "TestAnother", false},
 		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic", "acceptance", "TestAccept/bundle/templates/default-python/combinations/classic/x", false},
 
 		// pattern version
-		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic/", "acceptance", "TestAccept", true},
+		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic/", "acceptance", "TestAccept", false},
 		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic/", "acceptance", "TestAnother", false},
 		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic/", "acceptance", "TestAccept/bundle/templates/default-python/combinations/classic/x", true},
 		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic/", "acceptance", "TestAccept/bundle/templates/default-python/combinations/classic", true},
-		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic/", "acceptance", "TestAccept/bundle/templates/default-python/combinations", true},
+		{"acceptance TestAccept/bundle/templates/default-python/combinations/classic/", "acceptance", "TestAccept/bundle/templates/default-python/combinations", false},
 	}
 
 	for _, tt := range tests {
@@ -66,3 +71,69 @@ func TestConfigRuleMatches(t *testing.T) {
 		})
 	}
 }
+
+func TestCheckFailures(t *testing.T) {
+	const config = "* TestAccept/ssh/connection\n"
+
+	tests := []struct {
+		name     string
+		results  []TestResult
+		wantExit int
+	}{
+		{
+			// A parent failing because a listed subtest failed is allowed.
+			name: "parent allowed when subtest failed",
+			results: []TestResult{
+				{Action: "fail", Package: "acceptance", Test: "TestAccept/ssh/connection"},
+				{Action: "fail", Package: "acceptance", Test: "TestAccept"},
+			},
+			wantExit: 0,
+		},
+		{
+			// A parent failing on its own (no subtest failed) is not allowed,
+			// even though a subtest is listed as a known failure. This is the
+			// ruff-missing-in-setup case.
+			name: "parent not allowed without failing subtest",
+			results: []TestResult{
+				{Action: "fail", Package: "acceptance", Test: "TestAccept"},
+			},
+			wantExit: 7,
+		},
+		{
+			name: "unlisted failure is not allowed",
+			results: []TestResult{
+				{Action: "fail", Package: "acceptance", Test: "TestSomethingElse"},
+			},
+			wantExit: 7,
+		},
+		{
+			// gotestsum reruns failed tests; a failure followed by a pass is
+			// flaky, not a failure.
+			name: "failure passing on retry is allowed",
+			results: []TestResult{
+				{Action: "fail", Package: "acceptance", Test: "TestAccept"},
+				{Action: "pass", Package: "acceptance", Test: "TestAccept"},
+			},
+			wantExit: 0,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cfg, err := parseConfig(config)
+			require.NoError(t, err)
+
+			jsonFile := filepath.Join(t.TempDir(), "results.json")
+			var sb strings.Builder
+			for _, r := range tt.results {
+				line, err := json.Marshal(r)
+				require.NoError(t, err)
+				sb.Write(line)
+				sb.WriteByte('\n')
+			}
+			require.NoError(t, os.WriteFile(jsonFile, []byte(sb.String()), 0o600))
+
+			assert.Equal(t, tt.wantExit, checkFailures(cfg, jsonFile, 7))
+		})
+	}
+}