diff --git a/tools/testrunner/main.go b/tools/testrunner/main.go index f3ae8897da5..a27ffc3b8ad 100644 --- a/tools/testrunner/main.go +++ b/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 " ", 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 + "/" diff --git a/tools/testrunner/main_test.go b/tools/testrunner/main_test.go index fe4941e669a..780c1adaee2 100644 --- a/tools/testrunner/main_test.go +++ b/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)) + }) + } +}