[builder] Fetch active failures for a builder's configurations

The active failures for a builder's configurations will be fetched,
and a limited number of them will be reported in the build status.

Bug: b/210111991
Change-Id: Ie6b53ccc4c1872aa0024591f1bb5aa25ab4e0685
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/234302
Reviewed-by: Alexander Thomas <athom@google.com>
Commit-Queue: William Hesse <whesse@google.com>
diff --git a/builder/lib/src/builder.dart b/builder/lib/src/builder.dart
index b2a6dc0..24812f8 100644
--- a/builder/lib/src/builder.dart
+++ b/builder/lib/src/builder.dart
@@ -14,7 +14,7 @@
 import 'status.dart';
 import 'tryjob.dart' show Tryjob;
 
-/// A Builder holds information about a CI build, and can
+/// A Build holds information about a CI build, and can
 /// store the changed results from that build, using an injected
 /// [FirestoreService] object.
 /// Tryjob builds are represented by the class [Tryjob] instead.
@@ -60,6 +60,16 @@
     log('complete builder record');
     await firestore.completeBuilderRecord(info.builderName, endIndex, success);
     final status = BuildStatus()..success = success;
+    try {
+      status.unapprovedFailures = {
+        for (final configuration in configurations)
+          configuration: await unapprovedFailuresForConfiguration(configuration)
+      }..removeWhere((key, value) => value.isEmpty);
+    } catch (e) {
+      log('Failed to fetch unapproved failures: $e');
+      status.unapprovedFailures = {'failed': []};
+      status.success = false;
+    }
     final report = [
       'Processed ${changes.length} results from ${info.builderName}'
           ' build ${info.buildNumber}',
@@ -175,20 +185,39 @@
     if (failure && !approved) success = false;
 
     for (final activeResult in activeResults) {
-      final resultRecord = ResultRecord(activeResult.fields);
       // Log error message if any expected invariants are violated
-      if (resultRecord.blamelistEndIndex >= startIndex ||
-          !resultRecord.containsActiveConfiguration(change['configuration'])) {
-        // log('Unexpected active result when processing new change:\n'
-        //     'Active result: ${untagMap(activeResult.fields)}\n\n'
-        //     'Change: $change\n\n'
-        //     'approved: $approved');
+      if (activeResult.getInt(fBlamelistEndIndex)! >= startIndex ||
+          !(activeResult
+                  .getList(fActiveConfigurations)
+                  ?.contains(change['configuration']) ??
+              false)) {
+        log('Unexpected active result when processing new change:\n'
+            'Active result: ${untagMap(activeResult.fields)}\n\n'
+            'Change: $change\n\n'
+            'approved: $approved');
       }
       // Removes the configuration from the list of active configurations.
       await firestore.removeActiveConfiguration(
           activeResult, change['configuration']);
     }
   }
+
+  Future<List<SafeDocument>> unapprovedFailuresForConfiguration(
+      String configuration) async {
+    final failures = await firestore.findUnapprovedFailures(
+        configuration, BuildStatus.unapprovedFailuresLimit + 1);
+    await Future.forEach(failures, addBlamelistCommits);
+    return failures;
+  }
+
+  Future<void> addBlamelistCommits(SafeDocument result) async {
+    final startCommit = await commitsCache
+        .getCommitByIndex(result.getInt(fBlamelistStartIndex)!);
+    result.fields[fBlamelistStartCommit] = taggedValue(startCommit.hash);
+    final endCommit =
+        await commitsCache.getCommitByIndex(result.getInt(fBlamelistEndIndex)!);
+    result.fields[fBlamelistEndCommit] = taggedValue(endCommit.hash);
+  }
 }
 
 Map<String, dynamic> constructResult(
diff --git a/builder/lib/src/firestore.dart b/builder/lib/src/firestore.dart
index e0c32f7..467fc16 100644
--- a/builder/lib/src/firestore.dart
+++ b/builder/lib/src/firestore.dart
@@ -22,7 +22,7 @@
   Commit.fromJson(this.hash, Map<String, dynamic> data)
       : document = SafeDocument(Document(fields: taggedMap(data), name: ''));
   int get index => document.getInt('index')!;
-  String? get revertOf => document.getString(fRevertOf);
+  String? get revertOf => document.getStringOrNull(fRevertOf);
   bool get isRevert => document.fields.containsKey(fRevertOf);
   int? get review => document.getInt(fReview);
 
@@ -493,6 +493,18 @@
     return results;
   }
 
+  Future<List<SafeDocument>> findUnapprovedFailures(
+      String configuration, int limit) async {
+    final results = await query(
+        from: 'results',
+        where: compositeFilter([
+          arrayContains('active_configurations', configuration),
+          fieldEquals('approved', false)
+        ]),
+        limit: limit);
+    return results;
+  }
+
   Future<bool> hasReview(String review) async {
     return documentExists('$documents/reviews/$review');
   }
diff --git a/builder/lib/src/firestore_helpers.dart b/builder/lib/src/firestore_helpers.dart
index f0cd605..8c52d0a 100644
--- a/builder/lib/src/firestore_helpers.dart
+++ b/builder/lib/src/firestore_helpers.dart
@@ -21,7 +21,11 @@
     return int.parse(value);
   }
 
-  String? getString(String name) {
+  String getString(String name) {
+    return fields[name]!.stringValue!;
+  }
+
+  String? getStringOrNull(String name) {
     return fields[name]?.stringValue;
   }
 
diff --git a/builder/lib/src/result.dart b/builder/lib/src/result.dart
index d3eecf5..6eef883 100644
--- a/builder/lib/src/result.dart
+++ b/builder/lib/src/result.dart
@@ -5,34 +5,8 @@
 // Field names and helper functions for result documents and
 // commit documents from Firestore.
 
-import 'dart:convert' show jsonEncode;
-
 import 'package:googleapis/firestore/v1.dart' show Value;
 
-class ResultRecord {
-  final Map<String, Value> fields;
-
-  ResultRecord(this.fields);
-
-  bool get approved => fields['approved']!.booleanValue!;
-
-  @override
-  String toString() => jsonEncode(fields);
-
-  int get blamelistEndIndex {
-    return int.parse(fields['blamelist_end_index']!.integerValue!);
-  }
-
-  bool containsActiveConfiguration(String configuration) {
-    for (final value in fields['active_configurations']!.arrayValue!.values!) {
-      if (value.stringValue != null && value.stringValue == configuration) {
-        return true;
-      }
-    }
-    return false;
-  }
-}
-
 // Field names of Result document fields
 const fName = 'name';
 const fResult = 'result';
@@ -49,6 +23,26 @@
 const fActive = 'active';
 const fConfigurations = 'configurations';
 const fActiveConfigurations = 'active_configurations';
+// Fields added to a Result document by addBlamelistHashes()
+const fBlamelistStartCommit = 'blamelist_start_commit';
+const fBlamelistEndCommit = 'blamelist_end_commit';
+
+// Fields of a results.json change
+const fBuilderName = 'builder_name';
+const fBuildNumber = 'build_number';
+const fConfiguration = 'configuration';
+const fCommitHash = 'commit_hash';
+const fPreviousCommitHash = 'previous_commit_hash';
+
+// Field names of commit document fields
+const fHash = 'hash';
+const fIndex = 'index';
+const fAuthor = 'author';
+const fCreated = 'created';
+const fTitle = 'title';
+const fReview = 'review';
+const fRevertOf = 'revert_of';
+const fRelandOf = 'reland_of';
 
 bool isChangedResult(Map<String, dynamic> change) =>
     change[fChanged] && (!change[fFlaky] || !change[fPreviousFlaky]);
@@ -81,16 +75,6 @@
       fromStringOrValue(change[fExpected])
     ].join(' ');
 
-// Field names of commit document fields
-const fHash = 'hash';
-const fIndex = 'index';
-const fAuthor = 'author';
-const fCreated = 'created';
-const fTitle = 'title';
-const fReview = 'review';
-const fRevertOf = 'revert_of';
-const fRelandOf = 'reland_of';
-
 /// The information about a builder, taken from a Result object,
 /// that is needed to process the results
 class BuildInfo {
@@ -102,13 +86,13 @@
   final String? previousCommitHash;
 
   BuildInfo(Map<String, dynamic> result)
-      : builderName = result['builder_name'],
-        buildNumber = int.parse(result['build_number']),
-        commitRef = result['commit_hash'],
-        previousCommitHash = result['previous_commit_hash'];
+      : builderName = result[fBuilderName],
+        buildNumber = int.parse(result[fBuildNumber]),
+        commitRef = result[fCommitHash],
+        previousCommitHash = result[fPreviousCommitHash];
 
   factory BuildInfo.fromResult(Map<String, dynamic> result) {
-    final commitRef = result['commit_hash'];
+    final commitRef = result[fCommitHash];
     final match = commitRefRegExp.matchAsPrefix(commitRef);
     if (match == null) {
       return BuildInfo(result);
diff --git a/builder/lib/src/status.dart b/builder/lib/src/status.dart
index 306dc1e..dad3d53 100644
--- a/builder/lib/src/status.dart
+++ b/builder/lib/src/status.dart
@@ -11,7 +11,7 @@
   static const unapprovedFailuresLimit = 10;
   bool success = true;
   bool truncatedResults = false;
-  final Map<String, List<ResultRecord>> unapprovedFailures = {};
+  Map<String, List<SafeDocument>> unapprovedFailures = {};
 
   String toJson() {
     return jsonEncode({
@@ -23,7 +23,9 @@
 
   String unapprovedFailuresReport() {
     if (unapprovedFailures.isEmpty) return '';
-    String resultLine(ResultRecord result) => '';
+    if (unapprovedFailures.containsKey('failed')) {
+      return 'There are unapproved failures. Error fetching details';
+    }
     return [
       'There are unapproved failures',
       for (final entry in unapprovedFailures.entries) ...[
@@ -36,3 +38,16 @@
     ].join('\n');
   }
 }
+
+String resultLine(SafeDocument result) {
+  final name = result.getString(fName);
+  final previous = result.getString(fPreviousResult);
+  final current = result.getString(fResult);
+  final expected = result.getString(fExpected);
+  final start = result.getString(fBlamelistStartCommit);
+  final end = result.getString(fBlamelistEndCommit);
+  final range = start == end
+      ? start.substring(0, 6)
+      : '${start.substring(0, 6)}..${end.substring(0, 6)}';
+  return '    $name   ($previous -> $current , expected $expected ) at $range';
+}
diff --git a/builder/lib/src/tryjob.dart b/builder/lib/src/tryjob.dart
index f7bb77a..3a468b5 100644
--- a/builder/lib/src/tryjob.dart
+++ b/builder/lib/src/tryjob.dart
@@ -81,9 +81,8 @@
   }
 
   bool isNotLandedResult(Map<String, dynamic> change) {
-    return !lastLandedResultByName.containsKey(change[fName]) ||
-        change[fResult] !=
-            lastLandedResultByName[change[fName]]!.getString(fResult);
+    return change[fResult] !=
+        lastLandedResultByName[change[fName]]?.getString(fResult);
   }
 
   Future<BuildStatus> process(List<Map<String, dynamic>> results) async {
@@ -97,7 +96,7 @@
         landedResults = await fetchLandedResults(configuration);
         // Map will contain the last result with each name.
         lastLandedResultByName = {
-          for (final result in landedResults) result.getString(fName)!: result
+          for (final result in landedResults) result.getString(fName): result
         };
       }
       final changes =
diff --git a/builder/test/builder_test.dart b/builder/test/builder_test.dart
new file mode 100644
index 0000000..428458e
--- /dev/null
+++ b/builder/test/builder_test.dart
@@ -0,0 +1,210 @@
+// Copyright (c) 2022, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+import 'package:builder/src/builder.dart';
+import 'package:builder/src/commits_cache.dart';
+import 'package:builder/src/firestore.dart';
+import 'package:builder/src/result.dart';
+import 'package:googleapis/firestore/v1.dart';
+import 'package:googleapis_auth/auth_io.dart';
+import 'package:http/http.dart' as http;
+import 'package:test/test.dart';
+
+// These tests read and write data from the staging Firestore database.
+// They use existing commits and reviews, and add new results from
+// a new fake builder for new tests, where the builder and test names are unique
+// to this test code and the records for them are removed afterward.
+// The test cleanup function removes these records, even if tests fail.
+// Requires the environment variable GOOGLE_APPLICATION_CREDENTIALS
+// to point to a json key to a service account.
+// To run against the staging database, use a service account.
+// with write access to dart_ci_staging datastore.
+
+late FirestoreService firestore;
+late http.Client client;
+late CommitsCache commitsCache;
+// The real commits we will test on, fetched from Firestore
+const index = 81010;
+const previousIndex = index - 1;
+const previousBlamelistEnd = previousIndex - 1;
+const previousBlamelistStart = previousBlamelistEnd - 3;
+const previousBuildPreviousIndex = previousBlamelistStart - 1;
+late Commit commit;
+late Commit previousCommit;
+late Commit previousBlamelistEndCommit;
+late Commit previousBlamelistStartCommit;
+late Commit previousBuildPreviousCommit;
+
+final buildersToRemove = <String>{};
+final testsToRemove = <String>{};
+
+void registerChangeForDeletion(Map<String, dynamic> change) {
+  buildersToRemove.add(change['builder_name']!);
+  testsToRemove.add(change['name']!);
+}
+
+Future<void> removeBuildersAndResults() async {
+  Future<void> deleteDocuments(List<SafeDocument> documents) async {
+    for (final document in documents) {
+      await firestore.deleteDocument(document.name);
+    }
+  }
+
+  for (final test in testsToRemove) {
+    await deleteDocuments(await firestore.query(
+        from: 'results', where: fieldEquals(fName, test)));
+  }
+  for (final builder in buildersToRemove) {
+    await deleteDocuments(await firestore.query(
+        from: 'builds', where: fieldEquals('builder', builder)));
+  }
+}
+
+Future<void> loadCommits() async {
+  commit = await commitsCache.getCommitByIndex(index);
+  previousCommit = await commitsCache.getCommitByIndex(previousIndex);
+  previousBlamelistStartCommit =
+      await commitsCache.getCommitByIndex(previousBlamelistStart);
+  previousBlamelistEndCommit =
+      await commitsCache.getCommitByIndex(previousBlamelistEnd);
+  previousBuildPreviousCommit =
+      await commitsCache.getCommitByIndex(previousBuildPreviousIndex);
+}
+
+Build makeBuild(Map<String, dynamic> firstChange) =>
+    Build(BuildInfo.fromResult(firstChange), commitsCache, firestore);
+
+Map<String, dynamic> makeChange(String name, String result,
+    {bool flaky = false}) {
+  final results = result.split('/');
+  final previous = results[0];
+  final current = results[1];
+  final expected = results[2];
+  final change = {
+    fName: '${name}_test',
+    fConfiguration: '${name}_configuration',
+    'suite': 'unused_field',
+    'test_name': 'unused_field',
+    'time_ms': 2384,
+    fResult: current,
+    fPreviousResult: previous,
+    fExpected: expected,
+    fMatches: current == expected,
+    fChanged: current != previous,
+    fCommitHash: commit.hash,
+    'commit_time': 1583906489,
+    fBuildNumber: '99997',
+    fBuilderName: 'builder_$name',
+    fFlaky: flaky,
+    fPreviousFlaky: false,
+    fPreviousCommitHash: previousCommit.hash,
+    'previous_commit_time': 1583906489,
+    'bot_name': 'fake_bot_name',
+    'previous_build_number': '306',
+  };
+  registerChangeForDeletion(change);
+  return change;
+}
+
+Map<String, dynamic> makePreviousChange(String name, String result) {
+  return makeChange(name, result)
+    ..[fCommitHash] = previousBlamelistEndCommit.hash
+    ..[fPreviousCommitHash] = previousBuildPreviousCommit.hash;
+}
+
+void main() async {
+  final baseClient = http.Client();
+  client = await clientViaApplicationDefaultCredentials(
+      scopes: ['https://www.googleapis.com/auth/cloud-platform'],
+      baseClient: baseClient);
+  final api = FirestoreApi(client);
+  firestore = FirestoreService(api, client);
+  if (!await firestore.isStaging()) {
+    throw (TestFailure('Error: test is being run on production'));
+  }
+  commitsCache = CommitsCache(firestore, client);
+  await loadCommits();
+
+  tearDownAll(() async {
+    await removeBuildersAndResults();
+    baseClient.close();
+  });
+
+  test('existing failure', () async {
+    final failingPreviousChange =
+        makePreviousChange('failure', 'Pass/RuntimeError/Pass')
+          ..[fName] = 'previous_failure_test';
+    registerChangeForDeletion(failingPreviousChange); // Name changed.
+    final previousBuild = makeBuild(failingPreviousChange);
+    final previousStatus = await previousBuild.process([failingPreviousChange]);
+    final failingChange = makeChange('failure', 'Pass/RuntimeError/Pass');
+    final build = makeBuild(failingChange);
+    final status = await build.process([failingChange]);
+    expect(status.success, isFalse);
+    expect(status.truncatedResults, isFalse);
+    expect(status.unapprovedFailures, isNotEmpty);
+    expect(status.unapprovedFailures.keys, contains('failure_configuration'));
+    final failures = status.unapprovedFailures['failure_configuration']!;
+    final previousFailure = failures
+        .where((failure) => failure.getString(fName) == 'previous_failure_test')
+        .single;
+    final failure = failures
+        .where((failure) => failure.getString(fName) == 'failure_test')
+        .single;
+    expect(previousFailure.getStringOrNull(fBlamelistEndCommit),
+        previousBlamelistEndCommit.hash);
+    expect(previousFailure.getStringOrNull(fBlamelistStartCommit),
+        previousBlamelistStartCommit.hash);
+    expect(failure.getStringOrNull(fBlamelistEndCommit), commit.hash);
+    expect(failure.getStringOrNull(fBlamelistStartCommit), commit.hash);
+
+    final message = status.toJson();
+    expect(message, matches(r"There are unapproved failures\\n"));
+    expect(
+        message,
+        matches(
+            r'previous_failure_test   \(Pass -> RuntimeError , expected Pass \) at 44baaf\.\.ebe06b'));
+    expect(
+        message,
+        matches(
+            r"failure_test   \(Pass -> RuntimeError , expected Pass \) at 2368c2"));
+    expect(previousStatus.success, isFalse);
+    expect(previousStatus.unapprovedFailures.values.first, hasLength(1));
+  });
+
+  test('existing approved failure', () async {
+    final failingOtherConfigurationChange =
+        makeChange('other', 'Pass/RuntimeError/Pass')
+          ..[fName] = 'approved_failure_test'
+          ..[fPreviousCommitHash] = previousBuildPreviousCommit.hash;
+    registerChangeForDeletion(failingOtherConfigurationChange);
+    final otherConfigurationBuild = makeBuild(failingOtherConfigurationChange);
+    final otherStatus = await otherConfigurationBuild
+        .process([failingOtherConfigurationChange]);
+    final result = (await firestore.findActiveResults(
+            'approved_failure_test', 'other_configuration'))
+        .single;
+    expect(result.getInt(fBlamelistEndIndex), index);
+    expect(result.getInt(fBlamelistStartIndex), previousBlamelistStart);
+    await firestore.approveResult(result.toDocument());
+    final failingChange =
+        makeChange('approved_failure', 'Pass/RuntimeError/Pass');
+    final build = makeBuild(failingChange);
+    final status = await build.process([failingChange]);
+    expect(status.success, isTrue);
+    expect(status.truncatedResults, isFalse);
+    expect(status.unapprovedFailures, isEmpty);
+    expect(otherStatus.success, isFalse);
+    expect(otherStatus.unapprovedFailures, isNotEmpty);
+    expect(
+        otherStatus.unapprovedFailures.keys, contains('other_configuration'));
+    final changedResult = (await firestore.findActiveResults(
+            'approved_failure_test', 'other_configuration'))
+        .single;
+    expect(result.name, changedResult.name);
+    // Check blamelist narrowing.
+    expect(changedResult.getInt(fBlamelistEndIndex), index);
+    expect(changedResult.getInt(fBlamelistStartIndex), index);
+  });
+}
diff --git a/builder/test/revert_test.dart b/builder/test/revert_test.dart
index b41d560..fc61c7a 100644
--- a/builder/test/revert_test.dart
+++ b/builder/test/revert_test.dart
@@ -77,8 +77,8 @@
 
     Future<bool> findApproval(Map<String, dynamic> change) async {
       final result = await builderTest.firestore
-          .findActiveResults(change['name'], change['configuration']);
-      return ResultRecord(result.single.fields).approved;
+          .findActiveResults(change[fName], change[fConfiguration]);
+      return result.single.getBool(fApproved)!;
     }
 
     expect(await findApproval(commit56UnmatchingChange), false);