[cloud functions] Detect commits and reviews that are reverts
Saves a 'revert_of' field in the Firestore documents for commits and reviews
if they are a revert of a landed commit.
Change-Id: I2f66d3f2ceeb4dab9fbee2bcce9523be257194e2
Bug: https://github.com/dart-lang/dart_ci/issues/78
Reviewed-on: https://dart-review.googlesource.com/c/dart_ci/+/136420
Reviewed-by: Alexander Thomas <athom@google.com>
diff --git a/functions/node/builder.dart b/functions/node/builder.dart
index 51ce560..ba3204b 100644
--- a/functions/node/builder.dart
+++ b/functions/node/builder.dart
@@ -169,12 +169,14 @@
var index = lastIndex + 1;
for (Map<String, dynamic> commit in commits.reversed) {
final review = _review(commit);
+ final reverted = _revert(commit);
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 (review != null) 'review': review,
+ if (reverted != null) 'revert_of': reverted
});
if (review != null) {
await landReview(commit, index);
@@ -254,6 +256,12 @@
return null;
}
+final revertRegExp =
+ RegExp('^This reverts commit ([\\da-f]+)\\.\$', multiLine: true);
+
+String _revert(Map<String, dynamic> commit) =>
+ revertRegExp.firstMatch(commit['message'])?.group(1);
+
String testResult(Map<String, dynamic> change) => [
change['name'],
change['result'],
diff --git a/functions/node/gerrit_change.dart b/functions/node/gerrit_change.dart
index ec7a77a..263ad4a 100644
--- a/functions/node/gerrit_change.dart
+++ b/functions/node/gerrit_change.dart
@@ -10,6 +10,8 @@
class GerritInfo {
static const gerritUrl = 'https://dart-review.googlesource.com/changes';
+ static const gerritQuery =
+ 'o=ALL_REVISIONS&o=DETAILED_ACCOUNTS&o=CURRENT_COMMIT';
static const trivialKinds = const {
'TRIVIAL_REBASE',
'NO_CHANGE',
@@ -31,14 +33,18 @@
Future<void> update() async {
if (await firestore.hasPatchset(review, patchset)) return;
// Get the Gerrit change's commit from the Gerrit API.
- final url = '$gerritUrl/$review?o=ALL_REVISIONS&o=DETAILED_ACCOUNTS';
+ final url = '$gerritUrl/$review?$gerritQuery';
final response = await httpClient.get(url);
final protectedJson = response.body;
if (!protectedJson.startsWith(prefix))
throw Exception('Gerrit response missing prefix $prefix: $protectedJson');
final reviewInfo = jsonDecode(protectedJson.substring(prefix.length))
as Map<String, dynamic>;
- await firestore.storeReview(review, {'subject': reviewInfo['subject']});
+ final reverted = revert(reviewInfo);
+ await firestore.storeReview(review, {
+ 'subject': reviewInfo['subject'],
+ if (reverted != null) 'revert_of': reverted
+ });
// Add the patchset information to the patchsets subcollection.
final revisions = reviewInfo['revisions'].values.toList()
@@ -57,4 +63,12 @@
});
}
}
+
+ static String revert(Map<String, dynamic> reviewInfo) {
+ final current = reviewInfo['current_revision'];
+ final commit = reviewInfo['revisions'][current]['commit'];
+ final regExp =
+ RegExp('^This reverts commit ([\\da-f]+)\\.\$', multiLine: true);
+ return regExp.firstMatch(commit['message'])?.group(1);
+ }
}
diff --git a/functions/node/test/gerrit_review_json.dart b/functions/node/test/gerrit_review_json.dart
new file mode 100644
index 0000000..2da5c32
--- /dev/null
+++ b/functions/node/test/gerrit_review_json.dart
@@ -0,0 +1,109 @@
+// 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.
+
+// This is the JSON log for an actual review that is a revert,
+// with emails and names removed.
+String revertReviewGerritLog = r'''
+{
+ "id": "sdk~master~Ie212fae88bc1977e34e4d791c644b77783a8deb1",
+ "project": "sdk",
+ "branch": "master",
+ "hashtags": [],
+ "change_id": "Ie212fae88bc1977e34e4d791c644b77783a8deb1",
+ "subject": "Revert \"[SDK] Adds IndirectGoto implementation of sync-yield.\"",
+ "status": "MERGED",
+ "created": "2020-02-17 12:17:05.000000000",
+ "updated": "2020-02-17 12:17:25.000000000",
+ "submitted": "2020-02-17 12:17:25.000000000",
+ "submitter": {
+ "_account_id": 5260,
+ "name": "commit-bot@chromium.org",
+ "email": "commit-bot@chromium.org"
+ },
+ "insertions": 61,
+ "deletions": 155,
+ "total_comment_count": 0,
+ "unresolved_comment_count": 0,
+ "has_review_started": true,
+ "revert_of": 133586,
+ "submission_id": "136125",
+ "_number": 136125,
+ "owner": {
+ },
+ "current_revision": "82f3f81fc82d06c575b0137ddbe71408826d8b46",
+ "revisions": {
+ "82f3f81fc82d06c575b0137ddbe71408826d8b46": {
+ "kind": "REWORK",
+ "_number": 2,
+ "created": "2020-02-17 12:17:25.000000000",
+ "uploader": {
+ "_account_id": 5260,
+ "name": "commit-bot@chromium.org",
+ "email": "commit-bot@chromium.org"
+ },
+ "ref": "refs/changes/25/136125/2",
+ "fetch": {
+ "rpc": {
+ "url": "rpc://dart/sdk",
+ "ref": "refs/changes/25/136125/2"
+ },
+ "http": {
+ "url": "https://dart.googlesource.com/sdk",
+ "ref": "refs/changes/25/136125/2"
+ },
+ "sso": {
+ "url": "sso://dart/sdk",
+ "ref": "refs/changes/25/136125/2"
+ }
+ },
+ "commit": {
+ "parents": [
+ {
+ "commit": "d2d00ff357bd64a002697b3c96c92a0fec83328c",
+ "subject": "[cfe] Allow unassigned late local variables"
+ }
+ ],
+ "author": {
+ "name": "gerrit_user",
+ "email": "gerrit_user@example.com",
+ "date": "2020-02-17 12:17:25.000000000",
+ "tz": 0
+ },
+ "committer": {
+ "name": "commit-bot@chromium.org",
+ "email": "commit-bot@chromium.org",
+ "date": "2020-02-17 12:17:25.000000000",
+ "tz": 0
+ },
+ "subject": "Revert \"[SDK] Adds IndirectGoto implementation of sync-yield.\"",
+ "message": "Revert \"[SDK] Adds IndirectGoto implementation of sync-yield.\"\n\nThis reverts commit 7ed1690b4ed6b56bc818173dff41a7a2530991a2.\n\nReason for revert: Crashes precomp.\n\nOriginal change\u0027s description:\n\u003e [SDK] Adds IndirectGoto implementation of sync-yield.\n\u003e \n\u003e Sets a threshold of five continuations determining if the old\n\u003e if-else or the new igoto-based implementation will be used.\n\u003e Informal benchmarking on x64 and arm_x64 point towards the overhead\n\u003e of the igoto-based impl. dropping off around this point.\n\u003e \n\u003e Benchmarks of this CL (threshold\u003d5) show drastic improvement in\n\u003e Calls.IterableManualIterablePolymorphicManyYields of about ~35-65%\n\u003e across {dart,dart-aot}-{ia32,x64,armv7hf,armv8}.\n\u003e \n\u003e Bug: https://github.com/dart-lang/sdk/issues/37754\n\u003e Change-Id: I6e113f1f98e9ab0f994cf93004227d616e9e4d07\n\u003e Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/133586\n\u003e Commit-Queue: XXXXX \u003cxxxx@example.com\u003e\n\u003e Reviewed-by: xxxx \u003cxxxxxx@example.com\u003e\n\nChange-Id: Ie212fae88bc1977e34e4d791c644b77783a8deb1\nNo-Presubmit: true\nNo-Tree-Checks: true\nNo-Try: true\nBug: https://github.com/dart-lang/sdk/issues/37754\nReviewed-on: https://dart-review.googlesource.com/c/sdk/+/136125\nReviewed-by: XXX\nCommit-Queue: XXXXXX\n"
+ },
+ "description": "Rebase"
+ },
+ "8bae95c4001a0815e89ebc4c89dc5ad42337a01b": {
+ "kind": "REWORK",
+ "_number": 1,
+ "created": "2020-02-17 12:17:05.000000000",
+ "uploader": {
+ },
+ "ref": "refs/changes/25/136125/1",
+ "fetch": {
+ "rpc": {
+ "url": "rpc://dart/sdk",
+ "ref": "refs/changes/25/136125/1"
+ },
+ "http": {
+ "url": "https://dart.googlesource.com/sdk",
+ "ref": "refs/changes/25/136125/1"
+ },
+ "sso": {
+ "url": "sso://dart/sdk",
+ "ref": "refs/changes/25/136125/1"
+ }
+ }
+ }
+ },
+ "requirements": []
+}
+''';
diff --git a/functions/node/test/test.dart b/functions/node/test/test.dart
index 375ce23..e9f51e7 100644
--- a/functions/node/test/test.dart
+++ b/functions/node/test/test.dart
@@ -115,6 +115,42 @@
verifyNoMoreInteractions(firestore);
});
+ test('fetch commit that is a revert', () async {
+ final client = HttpClientMock();
+ final firestore = FirestoreServiceMock();
+ when(firestore.getCommit(existingCommitHash))
+ .thenAnswer((_) => Future.value(existingCommit));
+ final revertResponses = [null, revertCommit];
+ when(firestore.getCommit(revertCommitHash))
+ .thenAnswer((_) => Future(() => revertResponses.removeAt(0)));
+ when(firestore.getLastCommit()).thenAnswer(
+ (_) => Future(() => {...existingCommit, 'id': existingCommitHash}));
+
+ when(firestore.tryApprovals(any)).thenAnswer((_) => Future(() => []));
+ when(firestore.reviewIsLanded(any)).thenAnswer((_) => Future.value(true));
+
+ when(client.get(any))
+ .thenAnswer((_) => Future(() => ResponseFake(revertGitilesLog)));
+
+ final builder =
+ Build(revertCommitHash, revertCommitChange, null, firestore, client);
+ await builder.storeBuildCommitsInfo();
+ expect(builder.endIndex, revertCommit['index']);
+ expect(builder.startIndex, existingCommit['index'] + 1);
+ expect(builder.tryApprovals, {});
+ verifyInOrder([
+ verify(firestore.getCommit(revertCommitHash)).called(2),
+ verify(firestore.getLastCommit()),
+ verify(firestore.addCommit(revertCommitHash, revertCommit)),
+ 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.tryApprovals(revertReview))
+ ]);
+ verifyNoMoreInteractions(firestore);
+ });
+
test("copy approvals from try results", () async {
final firestore = FirestoreServiceMock();
final client = HttpClientMock();
diff --git a/functions/node/test/test_data.dart b/functions/node/test/test_data.dart
index 1f88a2a..4286fb1 100644
--- a/functions/node/test/test_data.dart
+++ b/functions/node/test/test_data.dart
@@ -28,11 +28,11 @@
};
const String existingCommitHash = 'an already existing commit hash';
-
+const int existingCommitIndex = 52;
Map<String, dynamic> existingCommit = {
'author': 'test_user@example.com',
'created': DateTime.parse('2019-11-22 22:19:00Z'),
- 'index': 52,
+ 'index': existingCommitIndex,
'title': 'A commit used for testing, with index 52'
};
@@ -45,19 +45,22 @@
};
const String commit53Hash = 'commit 53 landing CL 77779 hash';
+const int commit53Index = existingCommitIndex + 1;
Map<String, dynamic> commit53 = {
'author': 'user@example.com',
'created': DateTime.parse('2019-11-28 12:07:55 +0000'),
- 'index': 53,
+ 'index': commit53Index,
'title': 'A commit on the git log',
'review': 77779
};
const String landedCommitHash = 'a commit landing a CL hash';
+const int landedCommitIndex = commit53Index + 1;
+
Map<String, dynamic> landedCommit = {
'author': 'gerrit_user@example.com',
'created': DateTime.parse('2019-11-29 15:15:00Z'),
- 'index': 54,
+ 'index': landedCommitIndex,
'title': 'A commit used for testing tryjob approvals, with index 54',
'review': 44445
};
@@ -85,6 +88,44 @@
"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 = {
+ "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": false,
+ "bot_name": "luci-dart-try-xenial-70-8fkh",
+ "commit_hash": revertCommitHash,
+ "commit_time": 1563576771,
+ "build_number": "401",
+ "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
+};
+
const Map<String, dynamic> activeFailureResult = {
"name": "test_suite/active_failing_test",
"configurations": [testConfiguration, 'configuration 2', 'configuration 3'],
@@ -250,3 +291,22 @@
]
}
''';
+
+String revertGitilesLog = '''
+)]}'
+{
+ "log": [
+ {
+ "commit": "$revertCommitHash",
+ "parents": ["$existingCommitHash"],
+ "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"
+ }
+ ]
+}
+''';
diff --git a/functions/node/test/test_gerrit.dart b/functions/node/test/test_gerrit.dart
new file mode 100644
index 0000000..b42d139
--- /dev/null
+++ b/functions/node/test/test_gerrit.dart
@@ -0,0 +1,17 @@
+// 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 'dart:convert';
+
+import 'package:test/test.dart';
+
+import '../gerrit_change.dart';
+import 'gerrit_review_json.dart';
+
+void main() async {
+ test("get revert information from Gerrit log api output", () {
+ expect(GerritInfo.revert(json.decode(revertReviewGerritLog)),
+ "7ed1690b4ed6b56bc818173dff41a7a2530991a2");
+ });
+}
diff --git a/results_feed/doc/results_feed.proto b/results_feed/doc/results_feed.proto
index a19f535..0aab202 100644
--- a/results_feed/doc/results_feed.proto
+++ b/results_feed/doc/results_feed.proto
@@ -45,8 +45,8 @@
bool approved = 13; // May be missing in older records, interpreted as false.
}
-// Similar to a Result message, but representing a result of a try builder on the CQ.
-// There is no blamelist, instead, a patchset on a Gerrit CL describes the build.
+// Similar to a Result message, but representing results from a try builder.
+// There is no blamelist. Gerrit CL and patchset numbers identify the build.
message TryResult {
// Unique document ID created by Firestore for this document.
string id = 1;
@@ -115,7 +115,8 @@
string author = 3; // The author of the commit, as an email address.
google.protobuf.Timestamp created = 4;
string title = 5; // The first line of the commit message.
- int32 review = 6; // If the commit is a landed Gerrit review, its number.
+ 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.
}
// A map from configuration names to builder names.
@@ -129,6 +130,7 @@
message Review {
string id = 1; // The Gerrit review number (CL number)
string subject = 2; // The first line of the commit message.
+ string revert_of = 3; // Optional: the hash of the commit this review reverts.
message Patchset {
string id = 1; // The patchset number.
@@ -145,7 +147,7 @@
}
repeated Patchset patchsets = 3;
- int32 landed_index = 4; // When a review lands, this is the index of the landed commit.
+ int32 landed_index = 4; // Optional: The index of the commit landing the CL.
}
message Comment {
