[cloud functions] Copy tryjob approvals from the last patchset group only.
Pin CI results approved on a tryjob to the commit landing that CL.
Change-Id: Ie61228dff7f7f371503de6920c05f9d2de718350
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/131641
Reviewed-by: Alexander Thomas <athom@google.com>
diff --git a/functions/node/builder.dart b/functions/node/builder.dart
index 8e106b5..08854f1 100644
--- a/functions/node/builder.dart
+++ b/functions/node/builder.dart
@@ -58,7 +58,7 @@
int buildNumber;
int startIndex;
int endIndex;
- Set<String> tryApprovals = {};
+ Map<String, int> tryApprovals = {};
bool success = true; // Changed to false if any unapproved failure is seen.
int countChanges = 0;
int commitsFetched;
@@ -127,17 +127,17 @@
"built commit: $commitHash");
}
}
- 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));
+ Future<void> fetchApprovals(int review) async {
+ if (review != null) {
+ for (final result in await firestore.tryApprovals(review)) {
+ tryApprovals[testResult(result)] = review;
}
}
}
- await fetchApprovals(endCommit);
+ await fetchApprovals(endCommit['review']);
for (var index = startIndex; index < endIndex; ++index) {
- await fetchApprovals(await firestore.getCommitByIndex(index));
+ await fetchApprovals((await firestore.getCommitByIndex(index))['review']);
}
}
@@ -217,9 +217,10 @@
Map<String, dynamic> activeResult =
await firestore.findActiveResult(change);
if (result == null) {
- approved = tryApprovals.contains(testResult(change));
+ final approvingReview = tryApprovals[testResult(change)];
+ approved = approvingReview != null;
await firestore.storeResult(change, startIndex, endIndex,
- approved: approved, failure: failure);
+ approved: approved, review: approvingReview, failure: failure);
if (approved) {
countApprovalsCopied++;
if (countApprovalsCopied <= 10)
diff --git a/functions/node/firestore.dart b/functions/node/firestore.dart
index b054b9e..f54fc0f 100644
--- a/functions/node/firestore.dart
+++ b/functions/node/firestore.dart
@@ -28,7 +28,7 @@
Future<void> storeResult(
Map<String, dynamic> change, int startIndex, int endIndex,
- {bool approved, bool failure});
+ {bool approved, int review, bool failure});
Future<bool> updateResult(
String result, String configuration, int startIndex, int endIndex,
diff --git a/functions/node/firestore_impl.dart b/functions/node/firestore_impl.dart
index 8e9bf75..355bd42 100644
--- a/functions/node/firestore_impl.dart
+++ b/functions/node/firestore_impl.dart
@@ -133,7 +133,7 @@
Future<void> storeResult(
Map<String, dynamic> change, int startIndex, int endIndex,
- {bool approved, bool failure}) =>
+ {bool approved, int review, bool failure}) =>
firestore.collection('results').add(DocumentData.fromMap({
'name': change['name'],
'result': change['result'],
@@ -141,6 +141,7 @@
'expected': change['expected'],
'blamelist_start_index': startIndex,
'blamelist_end_index': endIndex,
+ if (startIndex != endIndex && approved) 'pinned_index': review,
'configurations': <String>[change['configuration']],
'approved': approved,
if (failure) 'active': true,
@@ -307,10 +308,19 @@
}
Future<List<Map<String, dynamic>>> tryApprovals(int review) async {
+ final patchsets = await firestore
+ .collection('reviews/$review/patchsets')
+ .orderBy('number', descending: true)
+ .limit(1)
+ .get();
+ final lastPatchsetGroup =
+ patchsets.documents.first.data.getInt('patchset_group');
+ print(lastPatchsetGroup);
QuerySnapshot approvals = await firestore
.collection('try_results')
.where('approved', isEqualTo: true)
.where('review', isEqualTo: review)
+ .where('patchset', isGreaterThanOrEqualTo: lastPatchsetGroup)
.get();
return [for (final document in approvals.documents) document.data.toMap()];
}
diff --git a/functions/node/test/firestore_test_nodejs.dart b/functions/node/test/firestore_test_nodejs.dart
index c475dc9..1e09c08 100644
--- a/functions/node/test/firestore_test_nodejs.dart
+++ b/functions/node/test/firestore_test_nodejs.dart
@@ -78,6 +78,73 @@
for (final doc in snapshot.documents) {
await doc.reference.delete();
}
+ snapshot =
+ await fs.firestore.collection('reviews/$testReview/patchsets').get();
+ for (final doc in snapshot.documents) {
+ await doc.reference.delete();
+ }
+ await fs.firestore.document('reviews/$testReview').delete();
+ });
+
+ test('approved try result fetching', () async {
+ await firestore.storeReview(testReview.toString(), {
+ 'subject': 'test review: approved try result fetching',
+ });
+ await firestore.storePatchset(testReview.toString(), 1, {
+ 'kind': 'REWORK',
+ 'description': 'Initial upload',
+ 'patchset_group': 1,
+ 'number': 1,
+ });
+ await firestore.storePatchset(testReview.toString(), 2, {
+ 'kind': 'REWORK',
+ 'description': 'change',
+ 'patchset_group': 2,
+ 'number': 2,
+ });
+ await firestore.storePatchset(testReview.toString(), 3, {
+ 'kind': 'NO_CODE_CHANGE',
+ 'description': 'Edit commit message',
+ 'patchset_group': 2,
+ 'number': 3,
+ });
+ final tryResult = {
+ 'review': testReview,
+ 'configuration': 'test_configuration',
+ 'name': 'test_suite/test_name',
+ 'patchset': 1,
+ 'result': 'RuntimeError',
+ 'expected': 'Pass',
+ 'previous_result': 'Pass',
+ };
+ await firestore.storeTryChange(tryResult, testReview, 1);
+ final tryResult2 = Map<String, dynamic>.from(tryResult);
+ tryResult2['patchset'] = 2;
+ tryResult2['name'] = 'test_suite/test_name_2';
+ await firestore.storeTryChange(tryResult2, testReview, 2);
+ tryResult['patchset'] = 3;
+ tryResult['name'] = 'test_suite/test_name';
+ tryResult['expected'] = 'CompileTimeError';
+ await firestore.storeTryChange(tryResult, testReview, 3);
+ // Set the results on patchsets 1 and 2 to approved.
+ final snapshot = await fs.firestore
+ .collection('try_results')
+ .where('approved', isEqualTo: false)
+ .where('review', isEqualTo: testReview)
+ .where('patchset', isLessThanOrEqualTo: 2)
+ .get();
+ for (final document in snapshot.documents) {
+ await document.reference
+ .updateData(UpdateData.fromMap({'approved': true}));
+ }
+
+ // Should return only the approved change on patchset 2,
+ // not the one on patchset 1 or the unapproved change on patchset 3.
+ final approvals = await firestore.tryApprovals(testReview);
+ tryResult2['configurations'] = [tryResult2['configuration']];
+ tryResult2['approved'] = true;
+ tryResult2.remove('configuration');
+ expect(approvals, [tryResult2]);
});
test('Test tryjob result processing', () async {
@@ -87,7 +154,7 @@
final previousFailingChange = Map<String, dynamic>.from(
tryjobFailingChange)
..addAll(
- {"commit_hash": testPreviousPatchsetPath, "build_number": "307"});
+ {'commit_hash': testPreviousPatchsetPath, 'build_number': '307'});
await Tryjob(testPreviousPatchsetPath, 1, firestore, null)
.process([previousFailingChange]);
var snapshot = await fs.firestore
diff --git a/functions/node/test/test.dart b/functions/node/test/test.dart
index f561385..acc0a4a 100644
--- a/functions/node/test/test.dart
+++ b/functions/node/test/test.dart
@@ -84,8 +84,10 @@
await builder.storeBuildCommitsInfo();
expect(builder.endIndex, landedCommit['index']);
expect(builder.startIndex, existingCommit['index'] + 1);
- expect(builder.tryApprovals,
- {testResult(tryjobResults[0]), testResult(tryjobResults[1])});
+ expect(builder.tryApprovals, {
+ testResult(tryjobResults[0]): 44445,
+ testResult(tryjobResults[1]): 77779
+ });
verifyInOrder([
verify(firestore.getCommit(landedCommitHash)).called(2),
verify(firestore.getLastCommit()),
@@ -146,6 +148,7 @@
firestore.findResult(any, 53, 54),
firestore.storeResult(any, 53, 54,
approved: argThat(isTrue, named: 'approved'),
+ review: argThat(equals(44445), named: 'review'),
failure: argThat(isTrue, named: 'failure'))
]);
});
