[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);