[cloud functions] Fetch tryjob approvals for all commits in blamelist.
Change-Id: I2ee8e6a50f4e711d8f374cb3fb064074ee0074ea
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/127901
Reviewed-by: Alexander Thomas <athom@google.com>
diff --git a/functions/node/builder.dart b/functions/node/builder.dart
index d8bd9fa..89cc1d4 100644
--- a/functions/node/builder.dart
+++ b/functions/node/builder.dart
@@ -58,7 +58,7 @@
int buildNumber;
int startIndex;
int endIndex;
- Set<String> tryApprovals;
+ Set<String> tryApprovals = {};
bool success = true; // Changed to false if any unapproved failure is seen.
int countChanges = 0;
int commitsFetched;
@@ -98,27 +98,34 @@
/// 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 commit = await firestore.getCommit(commitHash);
- if (commit == null) {
+ var endCommit = await firestore.getCommit(commitHash);
+ if (endCommit == null) {
await getMissingCommits();
- commit = await firestore.getCommit(commitHash);
- if (commit == null) {
+ endCommit = await firestore.getCommit(commitHash);
+ if (endCommit == null) {
error('Result received with unknown commit hash $commitHash');
}
}
- endIndex = commit['index'];
- if (commit.containsKey('review')) {
- tryApprovals = {
- for (final result in await firestore.tryApprovals(commit['review']))
- testResult(result)
- };
- }
+ endIndex = endCommit['index'];
// If this is a new builder, use the current commit as a trivial blamelist.
if (firstResult['previous_commit_hash'] == null) {
startIndex = endIndex;
} else {
- commit = await firestore.getCommit(firstResult['previous_commit_hash']);
- startIndex = commit['index'] + 1;
+ final startCommit =
+ await firestore.getCommit(firstResult['previous_commit_hash']);
+ startIndex = startCommit['index'] + 1;
+ }
+ Future<void> fetchApprovals(Map<String, dynamic> commit) async {
+ if (commit.containsKey('review')) {
+ for (final result in await firestore.tryApprovals(commit['review'])) {
+ tryApprovals.add(testResult(result));
+ }
+ }
+ }
+
+ await fetchApprovals(endCommit);
+ for (var index = startIndex; index < endIndex; ++index) {
+ await fetchApprovals(await firestore.getCommitByIndex(index));
}
}
@@ -195,7 +202,7 @@
var approved;
String result = await firestore.findResult(change, startIndex, endIndex);
if (result == null) {
- approved = tryApprovals?.contains(testResult(change)) ?? false;
+ approved = tryApprovals.contains(testResult(change));
await firestore.storeResult(change, startIndex, endIndex,
approved: approved);
} else {
diff --git a/functions/node/firestore.dart b/functions/node/firestore.dart
index 1bad425..9922606 100644
--- a/functions/node/firestore.dart
+++ b/functions/node/firestore.dart
@@ -11,6 +11,8 @@
Future<Map<String, dynamic>> getCommit(String hash);
+ Future<Map<String, dynamic>> getCommitByIndex(int index);
+
Future<Map<String, dynamic>> getLastCommit();
Future<void> addCommit(String id, Map<String, dynamic> data);
diff --git a/functions/node/firestore_impl.dart b/functions/node/firestore_impl.dart
index 636cd85..b6809ae 100644
--- a/functions/node/firestore_impl.dart
+++ b/functions/node/firestore_impl.dart
@@ -36,6 +36,12 @@
.get()
.then((s) => s.exists ? s.data.toMap() : null);
+ Future<Map<String, dynamic>> getCommitByIndex(int index) => firestore
+ .collection('commits')
+ .where('index', isEqualTo: index)
+ .get()
+ .then((s) => s.documents.first.data.toMap());
+
Future<Map<String, dynamic>> getLastCommit() async {
QuerySnapshot lastCommit = await firestore
.collection('commits')
diff --git a/functions/node/test/test.dart b/functions/node/test/test.dart
index bf6b27c..5dbb946 100644
--- a/functions/node/test/test.dart
+++ b/functions/node/test/test.dart
@@ -27,6 +27,13 @@
.thenAnswer((_) => Future.value(existingCommit));
when(firestore.getCommit(previousCommitHash))
.thenAnswer((_) => Future.value(previousCommit));
+ when(firestore.getCommitByIndex(any))
+ .thenAnswer((_) => Future.value(commit53));
+ when(firestore.tryApprovals(77779)).thenAnswer((_) => Future(() =>
+ tryjobResults
+ .where((result) => result['review'] == 77779)
+ .where((result) => result['approved'] == true)
+ .toList()));
final builder = Build(
existingCommitHash, existingCommitChange, null, firestore, client);
@@ -35,7 +42,11 @@
expect(builder.startIndex, previousCommit['index'] + 1);
verifyInOrder([
firestore.getCommit(existingCommitHash),
- firestore.getCommit(previousCommitHash)
+ firestore.getCommit(previousCommitHash),
+ firestore.getCommitByIndex(50),
+ firestore.tryApprovals(77779),
+ firestore.getCommitByIndex(51),
+ firestore.tryApprovals(77779),
]);
verifyNoMoreInteractions(firestore);
verifyZeroInteractions(client);
@@ -49,6 +60,8 @@
final landedResponses = [null, landedCommit];
when(firestore.getCommit(landedCommitHash))
.thenAnswer((_) => Future(() => landedResponses.removeAt(0)));
+ when(firestore.getCommitByIndex(53))
+ .thenAnswer((_) => Future.value(commit53));
when(firestore.getLastCommit()).thenAnswer(
(_) => Future(() => {...existingCommit, 'id': existingCommitHash}));
when(firestore.tryApprovals(44445)).thenAnswer((_) => Future(() =>
@@ -56,6 +69,11 @@
.where((result) => result['review'] == 44445)
.where((result) => result['approved'] == true)
.toList()));
+ when(firestore.tryApprovals(77779)).thenAnswer((_) => Future(() =>
+ tryjobResults
+ .where((result) => result['review'] == 77779)
+ .where((result) => result['approved'] == true)
+ .toList()));
when(firestore.reviewIsLanded(any)).thenAnswer((_) => Future.value(true));
when(client.get(any))
@@ -66,11 +84,12 @@
await builder.storeBuildCommitsInfo();
expect(builder.endIndex, landedCommit['index']);
expect(builder.startIndex, existingCommit['index'] + 1);
- expect(builder.tryApprovals, {testResult(tryjobResults[0])});
+ expect(builder.tryApprovals,
+ {testResult(tryjobResults[0]), testResult(tryjobResults[1])});
verifyInOrder([
verify(firestore.getCommit(landedCommitHash)).called(2),
verify(firestore.getLastCommit()),
- verify(firestore.addCommit(gitLogCommitHash, {
+ verify(firestore.addCommit(commit53Hash, {
'author': 'user@example.com',
'created': DateTime.parse('2019-11-28 12:07:55 +0000'),
'index': 53,
@@ -88,7 +107,10 @@
verify(firestore.reviewIsLanded(44445)),
// We want to verify firestore.getCommit(landedCommitHash) here,
// but it is already matched by the earlier check for the same call.
- verify(firestore.getCommit(existingCommitHash))
+ verify(firestore.getCommit(existingCommitHash)),
+ verify(firestore.tryApprovals(44445)),
+ verify(firestore.getCommitByIndex(53)),
+ verify(firestore.tryApprovals(77779))
]);
});
@@ -99,11 +121,18 @@
.thenAnswer((_) => Future.value(existingCommit));
when(firestore.getCommit(landedCommitHash))
.thenAnswer((_) => Future.value(landedCommit));
+ when(firestore.getCommitByIndex(53))
+ .thenAnswer((_) => Future.value(commit53));
when(firestore.tryApprovals(44445)).thenAnswer((_) => Future(() =>
tryjobResults
.where((result) => result['review'] == 44445)
.where((result) => result['approved'] == true)
.toList()));
+ when(firestore.tryApprovals(77779)).thenAnswer((_) => Future(() =>
+ tryjobResults
+ .where((result) => result['review'] == 77779)
+ .where((result) => result['approved'] == true)
+ .toList()));
final builder =
Build(landedCommitHash, landedCommitChange, null, firestore, client);
@@ -111,10 +140,10 @@
verifyZeroInteractions(client);
verifyInOrder([
- verify(firestore.updateConfiguration(
- "dart2js-new-rti-linux-x64-d8", "dart2js-rti-linux-x64-d8")),
- verify(firestore.findResult(any, 53, 54)),
- verify(firestore.storeResult(any, 53, 54, approved: true))
+ 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)
]);
});
}
diff --git a/functions/node/test/test_data.dart b/functions/node/test/test_data.dart
index 7912b35..3dd6f2f 100644
--- a/functions/node/test/test_data.dart
+++ b/functions/node/test/test_data.dart
@@ -44,6 +44,15 @@
'title': 'A commit used for testing, with index 49'
};
+const String commit53Hash = 'commit 53 landing CL 77779 hash';
+Map<String, dynamic> commit53 = {
+ 'author': 'user@example.com',
+ 'created': DateTime.parse('2019-11-28 12:07:55 +0000'),
+ 'index': 53,
+ 'title': 'A commit on the git log',
+ 'review': 77779
+};
+
const String landedCommitHash = 'a commit landing a CL hash';
Map<String, dynamic> landedCommit = {
'author': 'gerrit_user@example.com',
@@ -91,6 +100,16 @@
"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
+ },
];
const testBuilder = 'test_builder';
@@ -159,15 +178,13 @@
"changed": true
});
-String gitLogCommitHash = "a commit fetched from the git log";
-
String gitilesLog = '''
)]}'
{
"log": [
{
"commit": "$landedCommitHash",
- "parents": ["$gitLogCommitHash"],
+ "parents": ["$commit53Hash"],
"author": {
"email": "gerrit_user@example.com"
},
@@ -177,7 +194,7 @@
"message": "A commit used for testing tryjob approvals, with index 54\\n\\nDescription of the landed commit\\nin multiple lines.\\n\\u003e Change-Id: I8dcc08b7571137e869a16ceea8cc73539eb02a5a\\n\\u003e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/33337\\n\\nChange-Id: I89b88c3d9f7c743fc340ee73a45c3f57059bcf30\\nReviewed-on: https://dart-review.googlesource.com/c/sdk/+/44445\\n\\n"
},
{
- "commit": "$gitLogCommitHash",
+ "commit": "$commit53Hash",
"parents": ["$existingCommitHash"],
"author": {
"email": "user@example.com"
