dispatcher: fix test result parsing, build ranges for failure alerts, other fixes

go/src/infra/monitoring/analyzer/test_failures.go

Index: go/src/infra/monitoring/analyzer/test_failures.go
diff --git a/go/src/infra/monitoring/analyzer/test_failures.go b/go/src/infra/monitoring/analyzer/test_failures.go
index 5941420ce6c01de40163439b3239def933dce23a..205262c1a690e7782b196488b22e2ffa738eea5d 100644
--- a/go/src/infra/monitoring/analyzer/test_failures.go
+++ b/go/src/infra/monitoring/analyzer/test_failures.go
@@ -21,31 +21,86 @@ type TestFailureAnalyzer struct {
 // running.
 func (a *TestFailureAnalyzer) Analyze(f stepFailure) (*StepAnalyzerResult, error) {
 	ret := &StepAnalyzerResult{}
-	if !strings.HasSuffix(f.step.Name, "tests") {
+	// Some test steps have names like "webkit_tests iOS(dbug)" so we look at the first
+	// term before the space, if there is one.
+	s := strings.Split(f.step.Name, " ")
+	if !strings.HasSuffix(s[0], "tests") {
 		return ret, nil
 	}
 	ret.Recognized = true
 
 	testResults, err := a.Client.TestResults(f.masterName, f.builderName, f.step.Name, f.build.Number)
 	if err != nil {
+		ret.Reasons = append(ret.Reasons, f.step.Name)
 		return ret, fmt.Errorf("Error fetching test results: %v", err)
 	}
 
 	if len(testResults.Tests) == 0 {
-		log.Errorf("No test results for %v", f)
+		return ret, fmt.Errorf("No test results for %v", f)
 	}
-	log.Infof("%d test results", len(testResults.Tests))
 
 	for testName, testResults := range testResults.Tests {
-		// This string splitting logic was copied from builder_alerts.
-		// I'm not sure it's really necessary since the strings appear to always
-		// be either "PASS" or "FAIL" in practice.
-		expected := strings.Split(testResults.Expected, " ")
-		actual := strings.Split(testResults.Actual, " ")
-		ue := unexpected(expected, actual)
+		res, ok := testResults.(map[string]interface{})
+		if !ok {
+			log.Errorf("Couldn't convert test results to map: %s", testName)
+			continue
+		}
+
+		// If res is a simple top-level test result, just check it here.
+		if res["expected"] != nil || res["actual"] != nil {
+			expected := strings.Split(res["expected"].(string), " ")
+			actual := strings.Split(res["actual"].(string), " ")
+			ue := unexpected(expected, actual)
+			if len(ue) > 0 && res["bugs"] == nil {
+				ret.Reasons = append(ret.Reasons, testName)
+			}
+			continue
+		}
+
+		// res is not a simple top-level test result, so recurse to find
+		// the actual results.
+		ue, err := traverseResults(testName, res)
+		if err != nil {
+			return nil, err
+		}
 		if len(ue) > 0 {
+			// Don't give too many reasons, just the top level test/suite names.
+			// TODO: figure out a way to coalesce the unexpected results list into
+			// something compact but still meaningful in an alerting context.
 			ret.Reasons = append(ret.Reasons, testName)
 		}
 	}
 	return ret, nil
 }
+
+// testResults json is an arbitrarily deep tree, whose nodes are the actual
+// test results, so we recurse to find them.
+func traverseResults(parent string, testResults map[string]interface{}) ([]string, error) {
+	ret := []string{}
+	for testName, testResults := range testResults {
+		res, ok := testResults.(map[string]interface{})
+		if !ok {
+			return nil, fmt.Errorf("Couldn't convert test results to map: %s/%s", parent, testName)
+		}
+		// First check if results is actually results, or just another branch
+		// in the tree of test results
+		// Uuuuugly.
+		if res["expected"] == nil || res["actual"] == nil {
+			// Assume it's a branch.
+			res, err := traverseResults(fmt.Sprintf("%s/%s", parent, testName), res)
+			if err != nil {
+				return ret, err
+			}
+			ret = append(ret, res...)
+			continue
+		}
+
+		expected := strings.Split(res["expected"].(string), " ")
+		actual := strings.Split(res["actual"].(string), " ")
+		ue := unexpected(expected, actual)
+		if len(ue) > 0 && res["bugs"] == nil {
+			ret = append(ret, fmt.Sprintf("%s/%s: %+v vs %+v", parent, testName, expected, actual))
+		}
+	}
+	return ret, nil
+}