[cloud functions] Track the active failures, which have not been fixed.
Change-Id: Id1fe532101c5aba2b7553c5290a36ff78e8296af
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/128582
Reviewed-by: Alexander Thomas <athom@google.com>
diff --git a/functions/node/builder.dart b/functions/node/builder.dart
index 849844d..6836fa4 100644
--- a/functions/node/builder.dart
+++ b/functions/node/builder.dart
@@ -206,12 +206,15 @@
Future<void> storeChange(Map<String, dynamic> change) async {
countChanges++;
+ final failure = !change['matches'];
var approved;
String result = await firestore.findResult(change, startIndex, endIndex);
+ Map<String, dynamic> activeResult =
+ await firestore.findActiveResult(change);
if (result == null) {
approved = tryApprovals.contains(testResult(change));
await firestore.storeResult(change, startIndex, endIndex,
- approved: approved);
+ approved: approved, failure: failure);
if (approved) {
countApprovalsCopied++;
if (countApprovalsCopied <= 10)
@@ -220,9 +223,25 @@
}
} else {
approved = await firestore.updateResult(
- result, change['configuration'], startIndex, endIndex);
+ result, change['configuration'], startIndex, endIndex,
+ failure: failure);
}
- if (!approved && !change['matches']) success = false;
+ if (failure && !approved) success = false;
+
+ if (activeResult != null) {
+ // Log error message if any expected invariants are violated
+ if (activeResult['blamelist_end_index'] >= startIndex ||
+ !activeResult['active_configurations']
+ .contains(change['configuration'])) {
+ error('Unexpected active result when processing new change:\n'
+ 'Active result: $activeResult\n\n'
+ 'Change: $change\n\n'
+ 'approved: $approved');
+ }
+ // Removes the configuration from the list of active configurations.
+ // Mark the active result inactive when we remove the last active config.
+ firestore.updateActiveResult(activeResult, change['configuration']);
+ }
}
}
diff --git a/functions/node/firestore.dart b/functions/node/firestore.dart
index 1d5d397..292ccc4 100644
--- a/functions/node/firestore.dart
+++ b/functions/node/firestore.dart
@@ -26,15 +26,22 @@
Future<void> storeResult(
Map<String, dynamic> change, int startIndex, int endIndex,
- {bool approved = false});
+ {bool approved, bool failure});
Future<bool> updateResult(
- String result, String configuration, int startIndex, int endIndex);
+ String result, String configuration, int startIndex, int endIndex,
+ {bool failure});
Future<bool> storeTryChange(
Map<String, dynamic> change, int review, int patchset);
+ Future<void> updateActiveResult(
+ Map<String, dynamic> activeResult, String configuration);
+
+ Future<Map<String, dynamic>> findActiveResult(Map<String, dynamic> change);
+
Future<void> storeReview(String review, Map<String, dynamic> data);
+
Future<void> storePatchset(
String review, int patchset, Map<String, dynamic> data);
diff --git a/functions/node/firestore_impl.dart b/functions/node/firestore_impl.dart
index 06f3420..9c24675 100644
--- a/functions/node/firestore_impl.dart
+++ b/functions/node/firestore_impl.dart
@@ -120,7 +120,7 @@
Future<void> storeResult(
Map<String, dynamic> change, int startIndex, int endIndex,
- {bool approved = false}) =>
+ {bool approved, bool failure}) =>
firestore.collection('results').add(DocumentData.fromMap({
'name': change['name'],
'result': change['result'],
@@ -129,11 +129,15 @@
'blamelist_start_index': startIndex,
'blamelist_end_index': endIndex,
'configurations': <String>[change['configuration']],
- if (approved) 'approved': true
+ if (approved) 'approved': true,
+ if (failure) 'active': true,
+ if (failure)
+ 'active_configurations': <String>[change['configuration']]
}));
Future<bool> updateResult(
- String result, String configuration, int startIndex, int endIndex) {
+ String result, String configuration, int startIndex, int endIndex,
+ {bool failure}) {
DocumentReference reference = firestore.document('results/$result');
// Update the change group in a transaction.
@@ -151,6 +155,11 @@
});
update.setFieldValue('configurations',
Firestore.fieldValues.arrayUnion([configuration]));
+ if (failure) {
+ update.setBool('active', true);
+ update.setFieldValue('active_configurations',
+ Firestore.fieldValues.arrayUnion([configuration]));
+ }
reference.updateData(update);
return approved;
});
@@ -212,6 +221,40 @@
}
}
+ Future<void> updateActiveResult(
+ Map<String, dynamic> activeResult, String configuration) async {
+ final updateData = UpdateData();
+ if (activeResult['configurations'].length > 1) {
+ updateData.setFieldValue(
+ 'configurations', Firestore.fieldValues.arrayRemove([configuration]));
+ } else {
+ updateData.setFieldValue(
+ 'active_configurations', Firestore.fieldValues.delete());
+ updateData.setFieldValue('active', Firestore.fieldValues.delete());
+ }
+ return firestore
+ .document('results/${activeResult['id']}')
+ .updateData(updateData);
+ }
+
+ Future<Map<String, dynamic>> findActiveResult(
+ Map<String, dynamic> change) async {
+ QuerySnapshot snapshot = await firestore
+ .collection('results')
+ .where('active_configurations', arrayContains: change['configuration'])
+ .where('active', isEqualTo: true)
+ .where('name', isEqualTo: change['name'])
+ .get();
+ if (snapshot.isEmpty) return null;
+ if (snapshot.documents.length > 1) {
+ error('Multiple active results for the same configuration and test\n'
+ 'result 1: ${snapshot.documents[0].data.toMap()}\n'
+ 'result 2: ${snapshot.documents[1].data.toMap()}\n');
+ }
+ final result = snapshot.documents.first;
+ return result.data.toMap()..['id'] = result.documentID;
+ }
+
Future<void> storeReview(String review, Map<String, dynamic> data) =>
firestore.document('reviews/$review').setData(DocumentData.fromMap(data));
diff --git a/functions/node/test/test.dart b/functions/node/test/test.dart
index 5dbb946..46d6768 100644
--- a/functions/node/test/test.dart
+++ b/functions/node/test/test.dart
@@ -112,6 +112,7 @@
verify(firestore.getCommitByIndex(53)),
verify(firestore.tryApprovals(77779))
]);
+ verifyNoMoreInteractions(firestore);
});
test("copy approvals from try results", () async {
@@ -143,7 +144,51 @@
firestore.updateConfiguration(
"dart2js-new-rti-linux-x64-d8", "dart2js-rti-linux-x64-d8"),
firestore.findResult(any, 53, 54),
- firestore.storeResult(any, 53, 54, approved: true)
+ firestore.storeResult(any, 53, 54,
+ approved: argThat(isTrue, named: 'approved'),
+ failure: argThat(isTrue, named: 'failure'))
+ ]);
+ });
+
+ test("update previous active result", () async {
+ final activeResult = {
+ 'blamelist_end_index': 52,
+ 'configurations': [
+ landedCommitChange['configuration'],
+ 'another configuration'
+ ],
+ 'active_configurations': [
+ landedCommitChange['configuration'],
+ 'another configuration'
+ ],
+ 'active': true
+ };
+
+ final firestore = FirestoreServiceMock();
+ final client = HttpClientMock();
+
+ when(firestore.findActiveResult(any))
+ .thenAnswer((_) => Future.value(activeResult));
+
+ final builder =
+ Build(landedCommitHash, landedCommitChange, null, firestore, client);
+ builder.startIndex = 53;
+ builder.endIndex = 54;
+
+ await builder.storeChange(landedCommitChange);
+
+ expect(builder.countApprovalsCopied, 0);
+ expect(builder.countChanges, 1);
+ expect(builder.success, false);
+ verifyZeroInteractions(client);
+ verifyInOrder([
+ firestore.findResult(any, 53, 54),
+ firestore.findActiveResult(landedCommitChange),
+ firestore.storeResult(any, 53, 54,
+ approved: argThat(isFalse, named: 'approved'),
+ failure: argThat(isTrue, named: 'failure')),
+ firestore.updateActiveResult(
+ activeResult, landedCommitChange['configuration'])
]);
});
}
