Improve compare-failures output for multiple [BuildUri] results

tools/gardening/lib/src/compare_failures_impl.dart

Index: tools/gardening/lib/src/compare_failures_impl.dart
diff --git a/tools/gardening/lib/src/compare_failures_impl.dart b/tools/gardening/lib/src/compare_failures_impl.dart
index d3d77198061be4d9de8b975fb5ff855a0099b324..f52bb549d1e7dc19ea4aa3bb442d964f932b2eed 100644
--- a/tools/gardening/lib/src/compare_failures_impl.dart
+++ b/tools/gardening/lib/src/compare_failures_impl.dart
@@ -16,7 +16,7 @@ import 'package:gardening/src/util.dart';
 Future mainInternal(BuildbotClient client, List<String> args,
     {int runCount: 10}) async {
   printBuildResultsSummary(
-      await loadBuildResults(client, args, runCount: runCount));
+      await loadBuildResults(client, args, runCount: runCount), args);
 }
 
 /// Loads [BuildResult]s for the [runCount] last builds for the build(s) in
@@ -52,11 +52,55 @@ Future<Map<BuildUri, List<BuildResult>>> loadBuildResults(
 }
 
 /// Prints summaries for the [buildResults].
-// TODO(johnniwinther): Improve printing of multiple [BuildUri] results.
-void printBuildResultsSummary(Map<BuildUri, List<BuildResult>> buildResults) {
+void printBuildResultsSummary(
+    Map<BuildUri, List<BuildResult>> buildResults, List<String> args) {
+  List<Summary> emptySummaries = <Summary>[];
+  List<Summary> nonEmptySummaries = <Summary>[];
   buildResults.forEach((BuildUri buildUri, List<BuildResult> results) {
-    print(generateBuildResultsSummary(buildUri, results));
+    Summary summary = new Summary(buildUri, results);
+    if (summary.isEmpty) {
+      emptySummaries.add(summary);
+    } else {
+      nonEmptySummaries.add(summary);
+    }
   });
+  StringBuffer sb = new StringBuffer();
+  if (nonEmptySummaries.isEmpty) {
+    if (emptySummaries.isNotEmpty) {
+      if (LOG || emptySummaries.length < 3) {
+        if (emptySummaries.length == 1) {
+          sb.write('No errors found for build bot:\n');

[floitsch, 2017/08/08 13:42:02]

I would use "writeln"

here and in the other places that have a string that ends with "\n".

[Johnni Winther, 2017/08/08 14:23:30]

Done.

+          sb.write(emptySummaries.single.buildUri);
+        } else {
+          sb.write('No errors found for any of these build bots:\n');
+          for (Summary summary in emptySummaries) {
+            sb.write('${summary.buildUri}\n');
+          }
+        }
+      } else {
+        sb.write('No errors found for any of the '
+            '${emptySummaries.length} bots.');
+      }
+    } else {
+      sb.write('No build bot results found for args: ${args}');
+    }
+  } else {
+    for (Summary summary in nonEmptySummaries) {
+      summary.printOn(sb);
+    }
+    if (emptySummaries.isNotEmpty) {
+      if (LOG || emptySummaries.length < 3) {
+        sb.write('No errors found for the remaining build bots:\n');
+        for (Summary summary in emptySummaries) {
+          sb.write('${summary.buildUri}\n');
+        }
+      } else {
+        sb.write(
+            'No errors found for the ${emptySummaries.length} remaining bots.');
+      }
+    }
+  }
+  print(sb);
 }
 
 /// Creates a [BuildResult] for [buildUri] and, if it contains failures, the
@@ -76,89 +120,95 @@ Future<List<BuildResult>> readBuildResults(
   return summaries;
 }
 
-/// Generate a summary of the timeouts and other failures in [results].
-String generateBuildResultsSummary(
-    BuildUri buildUri, List<BuildResult> results) {
-  StringBuffer sb = new StringBuffer();
-  sb.write('Results for $buildUri:\n');
-  Set<TestConfiguration> timeoutIds = new Set<TestConfiguration>();
-  for (BuildResult result in results) {
-    timeoutIds.addAll(result.timeouts.map((TestFailure failure) => failure.id));
-  }
-  if (timeoutIds.isNotEmpty) {
-    Map<TestConfiguration, Map<int, Map<String, Timing>>> map =
-        <TestConfiguration, Map<int, Map<String, Timing>>>{};
-    Set<String> stepNames = new Set<String>();
+class Summary {
+  final BuildUri buildUri;
+  final List<BuildResult> results;
+  final Set<TestConfiguration> timeoutIds = new Set<TestConfiguration>();
+  final Set<TestConfiguration> errorIds = new Set<TestConfiguration>();
+
+  Summary(this.buildUri, this.results) {
     for (BuildResult result in results) {
-      for (Timing timing in result.timings) {
-        Map<int, Map<String, Timing>> builds =
-            map.putIfAbsent(timing.step.id, () => <int, Map<String, Timing>>{});
-        stepNames.add(timing.step.stepName);
-        builds.putIfAbsent(timing.uri.buildNumber, () => <String, Timing>{})[
-            timing.step.stepName] = timing;
-      }
+      timeoutIds
+          .addAll(result.timeouts.map((TestFailure failure) => failure.id));
+      errorIds.addAll(result.errors.map((TestFailure failure) => failure.id));
     }
-    sb.write('Timeouts for ${buildUri} :\n');
-    map.forEach((TestConfiguration id, Map<int, Map<String, Timing>> timings) {
-      if (!timeoutIds.contains(id)) return;
-      sb.write('$id\n');
-      sb.write(
-          '${' ' * 8} ${stepNames.map((t) => padRight(t, 14)).join(' ')}\n');
+  }
+
+  bool get isEmpty => timeoutIds.isEmpty && errorIds.isEmpty;
+
+  /// Generate a summary of the timeouts and other failures in [results].
+  void printOn(StringBuffer sb) {
+    if (timeoutIds.isNotEmpty) {
+      Map<TestConfiguration, Map<int, Map<String, Timing>>> map =
+          <TestConfiguration, Map<int, Map<String, Timing>>>{};
+      Set<String> stepNames = new Set<String>();
       for (BuildResult result in results) {
-        int buildNumber = result.buildUri.buildNumber;
-        Map<String, Timing> steps = timings[buildNumber] ?? const {};
-        sb.write(padRight(' ${buildNumber}: ', 8));
-        for (String stepName in stepNames) {
-          Timing timing = steps[stepName];
-          if (timing != null) {
-            sb.write(' ${timing.time}');
-          } else {
-            sb.write(' --------------');
+        for (Timing timing in result.timings) {
+          Map<int, Map<String, Timing>> builds = map.putIfAbsent(
+              timing.step.id, () => <int, Map<String, Timing>>{});
+          stepNames.add(timing.step.stepName);
+          builds.putIfAbsent(timing.uri.buildNumber, () => <String, Timing>{})[
+              timing.step.stepName] = timing;
+        }
+      }
+      sb.write('Timeouts for ${buildUri} :\n');
+      map.forEach(
+          (TestConfiguration id, Map<int, Map<String, Timing>> timings) {
+        if (!timeoutIds.contains(id)) return;
+        sb.write('$id\n');
+        sb.write(
+            '${' ' * 8} ${stepNames.map((t) => padRight(t, 14)).join(' ')}\n');
+        for (BuildResult result in results) {
+          int buildNumber = result.buildUri.buildNumber;
+          Map<String, Timing> steps = timings[buildNumber] ?? const {};
+          sb.write(padRight(' ${buildNumber}: ', 8));
+          for (String stepName in stepNames) {
+            Timing timing = steps[stepName];
+            if (timing != null) {
+              sb.write(' ${timing.time}');
+            } else {
+              sb.write(' --------------');
+            }
           }
+          sb.write('\n');
         }
         sb.write('\n');
-      }
-      sb.write('\n');
-    });
-  }
-  Set<TestConfiguration> errorIds = new Set<TestConfiguration>();
-  for (BuildResult result in results) {
-    errorIds.addAll(result.errors.map((TestFailure failure) => failure.id));
-  }
-  if (errorIds.isNotEmpty) {
-    Map<TestConfiguration, Map<int, TestFailure>> map =
-        <TestConfiguration, Map<int, TestFailure>>{};
-    for (BuildResult result in results) {
-      for (TestFailure failure in result.errors) {
-        map.putIfAbsent(failure.id, () => <int, TestFailure>{})[
-            failure.uri.buildNumber] = failure;
-      }
+      });
     }
-    sb.write('Errors for ${buildUri} :\n');
-    // TODO(johnniwinther): Improve comparison of non-timeouts.
-    map.forEach((TestConfiguration id, Map<int, TestFailure> failures) {
-      if (!errorIds.contains(id)) return;
-      sb.write('$id\n');
+    if (errorIds.isNotEmpty) {
+      Map<TestConfiguration, Map<int, TestFailure>> map =
+          <TestConfiguration, Map<int, TestFailure>>{};
       for (BuildResult result in results) {
-        int buildNumber = result.buildUri.buildNumber;
-        TestFailure failure = failures[buildNumber];
-        sb.write(padRight(' ${buildNumber}: ', 8));
-        if (failure != null) {
-          sb.write(padRight(failure.expected, 10));
-          sb.write(' / ');
-          sb.write(padRight(failure.actual, 10));
-        } else {
-          sb.write(' ' * 10);
-          sb.write(' / ');
-          sb.write(padRight('-- OK --', 10));
+        for (TestFailure failure in result.errors) {
+          map.putIfAbsent(failure.id, () => <int, TestFailure>{})[
+              failure.uri.buildNumber] = failure;
         }
-        sb.write('\n');
       }
-      sb.write('\n');
-    });
-  }
-  if (timeoutIds.isEmpty && errorIds.isEmpty) {
-    sb.write('No errors found.');
+      sb.write('Errors for ${buildUri} :\n');
+      // TODO(johnniwinther): Improve comparison of non-timeouts.
+      map.forEach((TestConfiguration id, Map<int, TestFailure> failures) {
+        if (!errorIds.contains(id)) return;
+        sb.write('$id\n');
+        for (BuildResult result in results) {
+          int buildNumber = result.buildUri.buildNumber;
+          TestFailure failure = failures[buildNumber];
+          sb.write(padRight(' ${buildNumber}: ', 8));
+          if (failure != null) {
+            sb.write(padRight(failure.expected, 10));
+            sb.write(' / ');
+            sb.write(padRight(failure.actual, 10));
+          } else {
+            sb.write(' ' * 10);
+            sb.write(' / ');
+            sb.write(padRight('-- OK --', 10));
+          }
+          sb.write('\n');
+        }
+        sb.write('\n');
+      });
+    }
+    if (timeoutIds.isEmpty && errorIds.isEmpty) {
+      sb.write('No errors found for ${buildUri}');
+    }
   }
-  return sb.toString();
 }