[cloud functions] Approve expected failures when landing a revert
Change-Id: I3eaf2c34ca0c5633c3753bc86eac401acf675f3f
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/136623
Reviewed-by: Jonas Termansen <sortie@google.com>
diff --git a/functions/node/builder.dart b/functions/node/builder.dart
index ba3204b..da11e17 100644
--- a/functions/node/builder.dart
+++ b/functions/node/builder.dart
@@ -8,11 +8,14 @@
import 'package:pool/pool.dart';
import 'firestore.dart';
+import 'result.dart';
+import 'reverted_changes.dart';
const prefix = ")]}'\n";
-bool isChangedResult(Map<String, dynamic> result) =>
- result['changed'] && !result['flaky'] && !result['previous_flaky'];
+Iterable<int> range(int start, int end) sync* {
+ for (int i = start; i < end; ++i) yield i;
+}
const months = const {
'Jan': '01',
@@ -51,7 +54,12 @@
int buildNumber;
int startIndex;
int endIndex;
+ Map<String, dynamic> endCommit;
+ Future<void> _awaitCommits;
+ List<Map<String, dynamic>> commits;
Map<String, int> tryApprovals = {};
+ List<RevertedChanges> allRevertedChanges = [];
+
bool success = true; // Changed to false if any unapproved failure is seen.
int countChanges = 0;
int commitsFetched;
@@ -98,7 +106,7 @@
/// Saves the commit indices of the start and end of the blamelist.
Future<void> storeBuildCommitsInfo() async {
// Get indices of change. Range includes startIndex and endIndex.
- var endCommit = await firestore.getCommit(commitHash);
+ endCommit = await firestore.getCommit(commitHash);
if (endCommit == null) {
await getMissingCommits();
endCommit = await firestore.getCommit(commitHash);
@@ -106,42 +114,54 @@
throw 'Result received with unknown commit hash $commitHash';
}
}
- endIndex = endCommit['index'];
+ endIndex = endCommit[fIndex];
// If this is a new builder, use the current commit as a trivial blamelist.
if (firstResult['previous_commit_hash'] == null) {
startIndex = endIndex;
} else {
final startCommit =
await firestore.getCommit(firstResult['previous_commit_hash']);
- startIndex = startCommit['index'] + 1;
+ startIndex = startCommit[fIndex] + 1;
if (startIndex > endIndex) {
throw ArgumentError("Results received with empty blamelist\n"
"previous commit: ${firstResult['previous_commit_hash']}\n"
"built commit: $commitHash");
}
}
- Future<void> fetchApprovals(int review, int index) async {
- if (review != null) {
- for (final result in await firestore.tryApprovals(review)) {
- tryApprovals[testResult(result)] = index;
- }
- }
- }
-
- await fetchApprovals(endCommit['review'], endIndex);
- for (var index = startIndex; index < endIndex; ++index) {
- await fetchApprovals(
- (await firestore.getCommitByIndex(index))['review'], index);
- }
}
+ /// This async function's implementation runs exactly once.
+ /// Later invocations return the same future returned by the first invocation.
+ Future<void> fetchReviewsAndReverts() => _awaitCommits ??= () async {
+ commits = [
+ for (var index = startIndex; index < endIndex; ++index)
+ await firestore.getCommitByIndex(index),
+ endCommit
+ ];
+ for (final commit in commits) {
+ final index = commit[fIndex];
+ final review = commit[fReview];
+ final reverted = commit[fRevertOf];
+ if (review != null) {
+ tryApprovals.addAll({
+ for (final result in await firestore.tryApprovals(review))
+ testResult(result): index
+ });
+ }
+ if (reverted != null) {
+ allRevertedChanges
+ .add(await getRevertedChanges(reverted, index, firestore));
+ }
+ }
+ }();
+
/// This function is idempotent, so every call of it should write the
/// same info to new Firestore documents. It is save to call multiple
/// times simultaneously.
Future<void> getMissingCommits() async {
final lastCommit = await firestore.getLastCommit();
final lastHash = lastCommit['id'];
- final lastIndex = lastCommit['index'];
+ final lastIndex = lastCommit[fIndex];
final logUrl = 'https://dart.googlesource.com/sdk/+log/';
final range = '$lastHash..master';
@@ -169,14 +189,26 @@
var index = lastIndex + 1;
for (Map<String, dynamic> commit in commits.reversed) {
final review = _review(commit);
- final reverted = _revert(commit);
+ var reverted = _revert(commit);
+ var relanded = _reland(commit);
+ if (relanded != null) {
+ reverted = null;
+ }
+ if (reverted != null) {
+ final revertedCommit = await firestore.getCommit(reverted);
+ if (revertedCommit != null && revertedCommit[fRevertOf] != null) {
+ reverted = null;
+ relanded = revertedCommit[fRevertOf];
+ }
+ }
await firestore.addCommit(commit['commit'], {
- 'author': commit['author']['email'],
- 'created': parseGitilesDateTime(commit['committer']['time']),
- 'index': index,
- 'title': commit['message'].split('\n').first,
- if (review != null) 'review': review,
- if (reverted != null) 'revert_of': reverted
+ fAuthor: commit['author']['email'],
+ fCreated: parseGitilesDateTime(commit['committer']['time']),
+ fIndex: index,
+ fTitle: commit['message'].split('\n').first,
+ if (review != null) fReview: review,
+ if (reverted != null) fRevertOf: reverted,
+ if (relanded != null) fRelandOf: relanded,
});
if (review != null) {
await landReview(commit, index);
@@ -204,18 +236,25 @@
Future<void> storeChange(Map<String, dynamic> change) async {
countChanges++;
- final failure = !change['matches'];
+ await fetchReviewsAndReverts();
+ final failure = isFailure(change);
var approved;
String result = await firestore.findResult(change, startIndex, endIndex);
List<Map<String, dynamic>> activeResults =
await firestore.findActiveResults(change);
if (result == null) {
- final approvingReviewLandedIndex = tryApprovals[testResult(change)];
- approved = approvingReviewLandedIndex != null;
- await firestore.storeResult(change, startIndex, endIndex,
+ final approvingIndex = tryApprovals[testResult(change)] ??
+ allRevertedChanges
+ .firstWhere(
+ (revertedChange) => revertedChange.approveRevert(change),
+ orElse: () => null)
+ ?.revertIndex;
+ approved = approvingIndex != null;
+ final newResult = constructResult(change, startIndex, endIndex,
approved: approved,
- landedReviewIndex: approvingReviewLandedIndex,
+ landedReviewIndex: approvingIndex,
failure: failure);
+ await firestore.storeResult(newResult);
if (approved) {
countApprovalsCopied++;
if (countApprovalsCopied <= 10)
@@ -231,8 +270,8 @@
for (final activeResult in activeResults) {
// Log error message if any expected invariants are violated
- if (activeResult['blamelist_end_index'] >= startIndex ||
- !activeResult['active_configurations']
+ if (activeResult[fBlamelistEndIndex] >= startIndex ||
+ !activeResult[fActiveConfigurations]
.contains(change['configuration'])) {
print('Unexpected active result when processing new change:\n'
'Active result: $activeResult\n\n'
@@ -262,9 +301,25 @@
String _revert(Map<String, dynamic> commit) =>
revertRegExp.firstMatch(commit['message'])?.group(1);
-String testResult(Map<String, dynamic> change) => [
- change['name'],
- change['result'],
- change['previous_result'] ?? 'new test',
- change['expected']
- ].join(' ');
+final relandRegExp =
+ RegExp('^This is a reland of ([\\da-f]+)\\.?\$', multiLine: true);
+
+String _reland(Map<String, dynamic> commit) =>
+ relandRegExp.firstMatch(commit['message'])?.group(1);
+
+Map<String, dynamic> constructResult(
+ Map<String, dynamic> change, int startIndex, int endIndex,
+ {bool approved, int landedReviewIndex, bool failure}) =>
+ {
+ fName: change[fName],
+ fResult: change[fResult],
+ fPreviousResult: change[fPreviousResult] ?? 'new test',
+ fExpected: change[fExpected],
+ fBlamelistStartIndex: startIndex,
+ fBlamelistEndIndex: endIndex,
+ if (startIndex != endIndex && approved) fPinnedIndex: landedReviewIndex,
+ fConfigurations: <String>[change['configuration']],
+ fApproved: approved,
+ if (failure) fActive: true,
+ if (failure) fActiveConfigurations: <String>[change['configuration']]
+ };
diff --git a/functions/node/firestore.dart b/functions/node/firestore.dart
index aa047db..adf47d7 100644
--- a/functions/node/firestore.dart
+++ b/functions/node/firestore.dart
@@ -26,14 +26,14 @@
Future<String> findResult(
Map<String, dynamic> change, int startIndex, int endIndex);
- Future<void> storeResult(
- Map<String, dynamic> change, int startIndex, int endIndex,
- {bool approved, int landedReviewIndex, bool failure});
+ Future<void> storeResult(Map<String, dynamic> result);
Future<bool> updateResult(
String result, String configuration, int startIndex, int endIndex,
{bool failure});
+ Future<List<Map<String, dynamic>>> findRevertedChanges(int index);
+
Future<bool> storeTryChange(
Map<String, dynamic> change, int review, int patchset);
diff --git a/functions/node/firestore_impl.dart b/functions/node/firestore_impl.dart
index 37d100b..64336f8 100644
--- a/functions/node/firestore_impl.dart
+++ b/functions/node/firestore_impl.dart
@@ -126,24 +126,8 @@
?.documentID;
}
- Future<void> storeResult(
- Map<String, dynamic> change, int startIndex, int endIndex,
- {bool approved, int landedReviewIndex, bool failure}) =>
- firestore.collection('results').add(DocumentData.fromMap({
- 'name': change['name'],
- 'result': change['result'],
- 'previous_result': change['previous_result'] ?? 'new test',
- 'expected': change['expected'],
- 'blamelist_start_index': startIndex,
- 'blamelist_end_index': endIndex,
- if (startIndex != endIndex && approved)
- 'pinned_index': landedReviewIndex,
- 'configurations': <String>[change['configuration']],
- 'approved': approved,
- if (failure) 'active': true,
- if (failure)
- 'active_configurations': <String>[change['configuration']]
- }));
+ Future<void> storeResult(Map<String, dynamic> result) =>
+ firestore.collection('results').add(DocumentData.fromMap(result));
Future<bool> updateResult(
String result, String configuration, int startIndex, int endIndex,
@@ -178,6 +162,24 @@
return firestore.runTransaction(updateGroup);
}
+ Future<List<Map<String, dynamic>>> findRevertedChanges(int index) async {
+ QuerySnapshot pinned = await firestore
+ .collection('results')
+ .where('pinned_index', isEqualTo: index)
+ .get();
+ QuerySnapshot unpinned = await firestore
+ .collection('results')
+ .where('blamelist_end_index', isEqualTo: index)
+ .get();
+ return [
+ for (final document in pinned.documents) document.data.toMap(),
+ for (final document in unpinned.documents)
+ if (document.data.getInt('blamelist_start_index') == index &&
+ document.data.getInt('pinned_index') == null)
+ document.data.toMap(),
+ ];
+ }
+
Future<bool> storeTryChange(
Map<String, dynamic> change, int review, int patchset) async {
String name = change['name'];
diff --git a/functions/node/result.dart b/functions/node/result.dart
new file mode 100644
index 0000000..31b6a83
--- /dev/null
+++ b/functions/node/result.dart
@@ -0,0 +1,43 @@
+// Copyright (c) 2020, 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.
+
+// Field names and helper functions for result documents and
+// commit documents from Firestore.
+
+const fName = 'name';
+const fResult = 'result';
+const fPreviousResult = 'previous_result';
+const fExpected = 'expected';
+const fChanged = 'changed';
+const fMatches = 'matches';
+const fFlaky = 'flaky';
+const fPreviousFlaky = 'previous_flaky';
+const fPinnedIndex = 'pinned_index';
+const fBlamelistStartIndex = 'blamelist_start_index';
+const fBlamelistEndIndex = 'blamelist_end_index';
+const fApproved = 'approved';
+const fActive = 'active';
+const fConfigurations = 'configurations';
+const fActiveConfigurations = 'active_configurations';
+
+bool isChangedResult(Map<String, dynamic> change) =>
+ change[fChanged] && !change[fFlaky] && !change[fPreviousFlaky];
+
+bool isFailure(Map<String, dynamic> change) => !change[fMatches];
+
+String testResult(Map<String, dynamic> change) => [
+ change[fName],
+ change[fResult],
+ change[fPreviousResult] ?? 'new test',
+ change[fExpected]
+ ].join(' ');
+
+// Fields for commit document
+const fIndex = 'index';
+const fAuthor = 'author';
+const fCreated = 'created';
+const fTitle = 'title';
+const fReview = 'review';
+const fRevertOf = 'revert_of';
+const fRelandOf = 'reland_of';
diff --git a/functions/node/reverted_changes.dart b/functions/node/reverted_changes.dart
new file mode 100644
index 0000000..68d7c94
--- /dev/null
+++ b/functions/node/reverted_changes.dart
@@ -0,0 +1,37 @@
+// Copyright (c) 2020, 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:collection/collection.dart';
+
+import 'firestore.dart';
+import 'result.dart';
+
+Future<RevertedChanges> getRevertedChanges(
+ String reverted, int revertIndex, FirestoreService firestore) async {
+ final revertedCommit = await firestore.getCommit(reverted);
+ if (revertedCommit == null) {
+ throw 'Cannot find commit for reverted commit hash $reverted';
+ }
+ final index = revertedCommit['index'];
+ final changes = await firestore.findRevertedChanges(index);
+ return RevertedChanges(
+ index, revertIndex, changes, groupBy(changes, (change) => change[fName]));
+}
+
+class RevertedChanges {
+ final int index;
+ final int revertIndex;
+ final List<Map<String, dynamic>> changes;
+ final Map<String, List<Map<String, dynamic>>> changesForTest;
+
+ RevertedChanges(
+ this.index, this.revertIndex, this.changes, this.changesForTest);
+
+ bool approveRevert(Map<String, dynamic> revert) {
+ final reverted = changesForTest[revert[fName]];
+ return isFailure(revert) &&
+ reverted != null &&
+ reverted.any((change) => revert[fResult] == change[fPreviousResult]);
+ }
+}
diff --git a/functions/node/test/test.dart b/functions/node/test/test.dart
index e9f51e7..7815bd2 100644
--- a/functions/node/test/test.dart
+++ b/functions/node/test/test.dart
@@ -8,6 +8,7 @@
import '../builder.dart';
import '../firestore.dart';
+import '../result.dart';
import 'test_data.dart';
class FirestoreServiceMock extends Mock implements FirestoreService {}
@@ -118,13 +119,13 @@
test('fetch commit that is a revert', () async {
final client = HttpClientMock();
final firestore = FirestoreServiceMock();
- when(firestore.getCommit(existingCommitHash))
- .thenAnswer((_) => Future.value(existingCommit));
+ when(firestore.getCommit(landedCommitHash))
+ .thenAnswer((_) => Future.value(landedCommit));
final revertResponses = [null, revertCommit];
when(firestore.getCommit(revertCommitHash))
.thenAnswer((_) => Future(() => revertResponses.removeAt(0)));
when(firestore.getLastCommit()).thenAnswer(
- (_) => Future(() => {...existingCommit, 'id': existingCommitHash}));
+ (_) => Future(() => {...landedCommit, 'id': landedCommitHash}));
when(firestore.tryApprovals(any)).thenAnswer((_) => Future(() => []));
when(firestore.reviewIsLanded(any)).thenAnswer((_) => Future.value(true));
@@ -133,10 +134,10 @@
.thenAnswer((_) => Future(() => ResponseFake(revertGitilesLog)));
final builder =
- Build(revertCommitHash, revertCommitChange, null, firestore, client);
+ Build(revertCommitHash, revertUnchangedChange, null, firestore, client);
await builder.storeBuildCommitsInfo();
expect(builder.endIndex, revertCommit['index']);
- expect(builder.startIndex, existingCommit['index'] + 1);
+ expect(builder.startIndex, landedCommit['index'] + 1);
expect(builder.tryApprovals, {});
verifyInOrder([
verify(firestore.getCommit(revertCommitHash)).called(2),
@@ -145,7 +146,7 @@
verify(firestore.reviewIsLanded(revertReview)),
// We want to verify firestore.getCommit(revertCommitHash) here,
// but it is already matched by the earlier check for the same call.
- verify(firestore.getCommit(existingCommitHash)),
+ verify(firestore.getCommit(landedCommitHash)),
verify(firestore.tryApprovals(revertReview))
]);
verifyNoMoreInteractions(firestore);
@@ -181,10 +182,7 @@
firestore.updateConfiguration(
"dart2js-new-rti-linux-x64-d8", "dart2js-rti-linux-x64-d8"),
firestore.findResult(any, 53, 54),
- firestore.storeResult(any, 53, 54,
- approved: argThat(isTrue, named: 'approved'),
- landedReviewIndex: argThat(equals(54), named: 'landedReviewIndex'),
- failure: argThat(isTrue, named: 'failure'))
+ firestore.storeResult(any),
]);
});
@@ -222,9 +220,7 @@
verifyInOrder([
firestore.findResult(any, 53, 54),
firestore.findActiveResults(landedCommitChange),
- firestore.storeResult(any, 53, 54,
- approved: argThat(isFalse, named: 'approved'),
- failure: argThat(isTrue, named: 'failure')),
+ firestore.storeResult(any),
firestore.updateActiveResult(
activeResult, landedCommitChange['configuration'])
]);
diff --git a/functions/node/test/test_data.dart b/functions/node/test/test_data.dart
index 4286fb1..abdbab8 100644
--- a/functions/node/test/test_data.dart
+++ b/functions/node/test/test_data.dart
@@ -4,27 +4,11 @@
import 'dart:core';
-const Map<String, dynamic> existingCommitChange = {
- "name": "dart2js_extra/local_function_signatures_strong_test/none",
- "configuration": "dart2js-new-rti-linux-x64-d8",
- "suite": "dart2js_extra",
- "test_name": "local_function_signatures_strong_test/none",
- "time_ms": 2384,
- "result": "Pass",
- "expected": "Pass",
- "matches": true,
- "bot_name": "luci-dart-try-xenial-70-8fkh",
- "commit_hash": existingCommitHash,
- "commit_time": 1563576771,
- "build_number": "307",
- "builder_name": "dart2js-rti-linux-x64-d8",
- "flaky": false,
- "previous_flaky": false,
- "previous_result": "RuntimeError",
- "previous_commit_hash": previousCommitHash,
- "previous_commit_time": 1563576211,
- "previous_build_number": "306",
- "changed": true
+Map<String, dynamic> fakeFirestoreCommits = {
+ existingCommitHash: existingCommit,
+ previousCommitHash: previousCommit,
+ commit53Hash: commit53,
+ landedCommitHash: landedCommit
};
const String existingCommitHash = 'an already existing commit hash';
@@ -54,6 +38,9 @@
'review': 77779
};
+const String newCommitHash = landedCommitHash;
+Map<String, dynamic> newCommitChange = landedCommitChange;
+
const String landedCommitHash = 'a commit landing a CL hash';
const int landedCommitIndex = commit53Index + 1;
@@ -65,6 +52,52 @@
'review': 44445
};
+const String revertCommitHash = 'commit reverting commit 48 hash';
+const int revertReview = 3426;
+const int revertIndex = landedCommitIndex + 1;
+Map<String, dynamic> revertCommit = {
+ 'author': 'gerrit_revert_user@example.com',
+ 'created': DateTime.parse('2019-11-29 16:15:00Z'),
+ 'index': revertIndex,
+ 'title': 'A commit reverting commit 48, with index 55',
+ 'review': revertReview,
+ 'revert_of': revertedCommitHash,
+};
+
+const String revertedCommitHash = '48abcd55abcd';
+const int revertedReview = 3926;
+const int revertedIndex = 48;
+Map<String, dynamic> revertedCommit = {
+ 'author': 'gerrit_reverted_user@example.com',
+ 'created': DateTime.parse('2019-11-22 02:01:00Z'),
+ 'index': revertedIndex,
+ 'title': 'A commit reverted by commit 55, with index 48',
+ 'review': revertedReview,
+};
+
+const Map<String, dynamic> existingCommitChange = {
+ "name": "dart2js_extra/local_function_signatures_strong_test/none",
+ "configuration": "dart2js-new-rti-linux-x64-d8",
+ "suite": "dart2js_extra",
+ "test_name": "local_function_signatures_strong_test/none",
+ "time_ms": 2384,
+ "result": "Pass",
+ "expected": "Pass",
+ "matches": true,
+ "bot_name": "luci-dart-try-xenial-70-8fkh",
+ "commit_hash": existingCommitHash,
+ "commit_time": 1563576771,
+ "build_number": "307",
+ "builder_name": "dart2js-rti-linux-x64-d8",
+ "flaky": false,
+ "previous_flaky": false,
+ "previous_result": "RuntimeError",
+ "previous_commit_hash": previousCommitHash,
+ "previous_commit_time": 1563576211,
+ "previous_build_number": "306",
+ "changed": true
+};
+
const Map<String, dynamic> landedCommitChange = {
"name": "dart2js_extra/local_function_signatures_strong_test/none",
"configuration": "dart2js-new-rti-linux-x64-d8",
@@ -88,22 +121,9 @@
"changed": true
};
-const String revertCommitHash = '23456ababa23456ababa';
-const String revertedHash = '012345abcde012345abcde';
-const int revertReview = 85937;
-const int revertCommitIndex = existingCommitIndex + 1;
-Map<String, dynamic> revertCommit = {
- 'author': 'gerrit_user@example.com',
- 'created': DateTime.parse('2019-11-29 15:15:00 +0000'),
- 'index': revertCommitIndex,
- 'title': 'A commit that reverts another commit',
- 'review': revertReview,
- 'revert_of': revertedHash,
-};
-
// This change is an unchanged passing result, used as the first result in
// a chunk with no changed results.
-const Map<String, dynamic> revertCommitChange = {
+const Map<String, dynamic> revertUnchangedChange = {
"name": "dart2js_extra/local_function_signatures_strong_test/none",
"configuration": "dart2js-new-rti-linux-x64-d8",
"suite": "dart2js_extra",
@@ -116,14 +136,82 @@
"commit_hash": revertCommitHash,
"commit_time": 1563576771,
"build_number": "401",
+ "previous_build_number": "400",
+ "changed": false,
+};
+
+const Map<String, dynamic> revertChange = {
+ "name": "test_suite/fixed_broken_test",
+ "configuration": "a_different_configuration",
+ "suite": "test_suite",
+ "test_name": "fixed_broken_test",
+ "time_ms": 2384,
+ "result": "RuntimeError",
+ "expected": "Pass",
+ "matches": false,
+ "bot_name": "a_ci_bot",
+ "commit_hash": revertCommitHash,
+ "commit_time": 1563576771,
+ "build_number": "314",
"builder_name": "dart2js-rti-linux-x64-d8",
"flaky": false,
"previous_flaky": false,
"previous_result": "Pass",
"previous_commit_hash": existingCommitHash,
"previous_commit_time": 1563576211,
- "previous_build_number": "400",
- "changed": false
+ "previous_build_number": "313",
+ "changed": true,
+};
+
+const Map<String, dynamic> revertResult = {
+ "configurations": ["a_different_configuration"],
+ "active": true,
+ "active_configurations": ["a_different_configuration"],
+ "name": "test_suite/fixed_broken_test",
+ "result": "RuntimeError",
+ "expected": "Pass",
+ "previous_result": "Pass",
+ "blamelist_start_index": 53,
+ "blamelist_end_index": 55,
+ "pinned_index": 55,
+ "approved": true,
+};
+
+const Map<String, dynamic> revertedChange = {
+ "name": "test_suite/fixed_broken_test",
+ "configuration": "a_configuration",
+ "suite": "test_suite",
+ "test_name": "fixed_broken_test",
+ "time_ms": 2384,
+ "result": "Pass",
+ "expected": "Pass",
+ "matches": true,
+ "bot_name": "a_ci_bot",
+ "commit_hash": revertCommitHash,
+ "commit_time": 1563576771,
+ "build_number": "308",
+ "builder_name": "dart2js-rti-linux-x64-d8",
+ "flaky": false,
+ "previous_flaky": false,
+ "previous_result": "RuntimeError",
+ "previous_commit_hash": "a nonexistent hash",
+ "previous_commit_time": 1563576211,
+ "previous_build_number": "306",
+ "changed": true
+};
+
+const Map<String, dynamic> revertedResult = {
+ "configurations": ["a_configuration"],
+ "name": "test_suite/fixed_broken_test",
+ "result": "Pass",
+ "expected": "Pass",
+ "previous_result": "RuntimeError",
+ "blamelist_start_index": 48,
+ "blamelist_end_index": 48,
+};
+
+Map<String, Map<String, dynamic>> fakeFirestoreResults = {
+ 'activeFailureResultID': activeFailureResult
};
const Map<String, dynamic> activeFailureResult = {
@@ -132,7 +220,6 @@
"active": true,
"active_configurations": [testConfiguration, 'configuration 2'],
"approved": false,
- "suite": "test_suite",
"result": "RuntimeError",
"expected": "Pass",
"previous_result": "Pass",
@@ -141,31 +228,33 @@
};
const List<Map<String, dynamic>> tryjobResults = [
- {
- "review": 44445,
- "configurations": [
- "dart2js-new-rti-linux-x64-d8",
- "dartk-reload-rollback-linux-debug-x64",
- "dartk-reload-linux-debug-x64"
- ],
- "name": "dart2js_extra/local_function_signatures_strong_test/none",
- "patchset": 1,
- "result": "RuntimeError",
- "expected": "Pass",
- "previous_result": "Pass",
- "approved": true
- },
- {
- "review": 77779,
- "configurations": ["test_configuration"],
- "name": "test_suite/test_name",
- "patchset": 5,
- "result": "RuntimeError",
- "expected": "CompileTimeError",
- "previous_result": "CompileTimeError",
- "approved": true
- },
+ review44445Result,
+ review77779Result
];
+const Map<String, dynamic> review44445Result = {
+ "review": 44445,
+ "configurations": [
+ "dart2js-new-rti-linux-x64-d8",
+ "dartk-reload-rollback-linux-debug-x64",
+ "dartk-reload-linux-debug-x64"
+ ],
+ "name": "dart2js_extra/local_function_signatures_strong_test/none",
+ "patchset": 1,
+ "result": "RuntimeError",
+ "expected": "Pass",
+ "previous_result": "Pass",
+ "approved": true
+};
+const Map<String, dynamic> review77779Result = {
+ "review": 77779,
+ "configurations": ["test_configuration"],
+ "name": "test_suite/test_name",
+ "patchset": 5,
+ "result": "RuntimeError",
+ "expected": "CompileTimeError",
+ "previous_result": "CompileTimeError",
+ "approved": true
+};
const testBuilder = 'test_builder';
const testBuildNumber = "308";
@@ -298,14 +387,14 @@
"log": [
{
"commit": "$revertCommitHash",
- "parents": ["$existingCommitHash"],
+ "parents": ["$landedCommitHash"],
"author": {
"email": "gerrit_user@example.com"
},
"committer": {
"time": "Fri Nov 29 15:15:00 2019 +0000"
},
- "message": "A commit that reverts another commit\\n\\nThis reverts commit $revertedHash.\\nChange-Id: I89b88c3d9f7c743fc340ee73a45c3f57059bcf30\\nReviewed-on: https://dart-review.googlesource.com/c/sdk/+/$revertReview\\n\\n"
+ "message": "A commit that reverts another commit\\n\\nThis reverts commit $revertedCommitHash.\\nChange-Id: I89b88c3d9f7c743fc340ee73a45c3f57059bcf30\\nReviewed-on: https://dart-review.googlesource.com/c/sdk/+/$revertReview\\n\\n"
}
]
}
diff --git a/functions/node/test/test_revert.dart b/functions/node/test/test_revert.dart
new file mode 100644
index 0000000..ca767de
--- /dev/null
+++ b/functions/node/test/test_revert.dart
@@ -0,0 +1,122 @@
+// Copyright (c) 2020, 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.
+
+// Tests that check automatic approval of failures on a revert on the CI
+
+import 'package:collection/collection.dart';
+import 'package:http/http.dart';
+import 'package:mockito/mockito.dart';
+import 'package:test/test.dart';
+
+import '../builder.dart';
+import '../firestore.dart';
+import '../result.dart';
+import 'test_data.dart';
+
+Future<void> main() async {
+ test("Base builder test", () async {
+ final builderTest = BuilderTest(newCommitHash, newCommitChange);
+ await builderTest.update();
+ });
+
+ test("Automatically approve expected failure on revert", () async {
+ final builderTest = RevertBuilderTest(revertCommitHash, revertChange);
+ await builderTest.update();
+ await builderTest.storeChange(revertChange);
+ expect(
+ builderTest.firestore.results.values
+ .where((result) => result[fBlamelistEndIndex] == 55)
+ .single,
+ revertResult);
+ });
+}
+
+class BuilderTest {
+ final client = HttpClientMock();
+ final firestore = FirestoreServiceFake();
+ Build builder;
+ final String commitHash;
+ final Map<String, dynamic> firstChange;
+
+ BuilderTest(this.commitHash, this.firstChange) {
+ builder = Build(commitHash, firstChange, null, firestore, client);
+ }
+
+ Future<void> storeBuildCommitsInfo() async {
+ await builder.storeBuildCommitsInfo();
+ // Test expectations
+ }
+
+ Future<void> storeChange(Map<String, dynamic> change) async {
+ return builder.storeChange(change);
+ }
+
+ Future<void> update() async {
+ await storeBuildCommitsInfo();
+ }
+}
+
+class FirestoreServiceFake extends Fake implements FirestoreService {
+ final Map<String, Map<String, dynamic>> commits =
+ Map.from(fakeFirestoreCommits);
+ final Map<String, Map<String, dynamic>> results =
+ Map.from(fakeFirestoreResults);
+ int _addedResultID = 0;
+
+ Future<void> addCommit(String id, Map<String, dynamic> data) async {
+ commits[id] = data;
+ }
+
+ Future<List<Map<String, dynamic>>> findActiveResults(
+ Map<String, dynamic> change) =>
+ Future.value([]);
+
+ Future<String> findResult(
+ Map<String, dynamic> change, int startIndex, int endIndex) =>
+ Future.value(null);
+
+ Future<List<Map<String, dynamic>>> findRevertedChanges(int index) =>
+ Future.value(results.values
+ .where((change) =>
+ change[fPinnedIndex] == index ||
+ (change[fBlamelistStartIndex] == index &&
+ change[fBlamelistEndIndex] == index))
+ .toList());
+
+ Future<Map<String, dynamic>> getCommit(String hash) =>
+ Future.value(commits[hash]);
+
+ Future<Map<String, dynamic>> getCommitByIndex(int index) =>
+ Future.value(commits.values
+ .firstWhere((commit) => commit[fIndex] == index, orElse: () => null));
+
+ Future<Map<String, dynamic>> getLastCommit() =>
+ Future.value(maxBy<Map<String, dynamic>, int>(
+ commits.values, (commit) => commit[fIndex]));
+
+ Future<void> storeResult(Map<String, dynamic> result) async {
+ final id = 'resultDocumentID$_addedResultID';
+ _addedResultID++;
+ results[id] = result;
+ }
+
+ Future<List<Map<String, dynamic>>> tryApprovals(int review) =>
+ Future.value([]);
+}
+
+class HttpClientMock extends Mock implements BaseClient {}
+
+class ResponseFake extends Fake implements Response {
+ String body;
+ ResponseFake(this.body);
+}
+
+class RevertBuilderTest extends BuilderTest {
+ RevertBuilderTest(String commitHash, Map<String, dynamic> firstChange)
+ : super(commitHash, firstChange) {
+ firestore.commits[revertCommitHash] = revertCommit;
+ firestore.commits[revertedCommitHash] = revertedCommit;
+ firestore.results['revertedResultID'] = revertedResult;
+ }
+}
diff --git a/results_feed/doc/results_feed.proto b/results_feed/doc/results_feed.proto
index 0aab202..fdb82f0 100644
--- a/results_feed/doc/results_feed.proto
+++ b/results_feed/doc/results_feed.proto
@@ -117,6 +117,7 @@
string title = 5; // The first line of the commit message.
int32 review = 6; // Optional: the id of the review this commit lands.
string revert_of = 7; // Optional: the hash of the commit this commit reverts.
+ string reland_of = 8; // Optional: the hash of the relanded commit.
}
// A map from configuration names to builder names.
