Fix DownstreamCreatorIT for NoteDb
One semantic difference between ReviewDb and NoteDb is that NoteDb does
not always create implicit 0 votes when a reviewer is added. These tests
were checking for a 0 vote primarily in order to verify the
autogenerated tags; a more direct way of testing this is checking for
the actual ChangeMessage we expect the plugin to create. Another
consequence is that some votes are missing rather than being zero, so
fix those asserts.
A final very non-obvious issue is that ChangeApi instances are generally
not safe to reuse in the presence of mutations behind their back.
Sometimes it works and sometimes it doesn't; it happens that it worked
more often under ReviewDb than under NoteDb. Fix this by extracting more
helper methods and re-reading the change given an ID, rather than just
reusing a ChangeApi instance.
Note that these fixes are purely to make the tests pass under NoteDb.
The plugin has actually been running exclusively under NoteDb on
googlesource.com hosts for almost two years, so we are reasonably
confident that the updated semantics described in the tests are
acceptable.
Change-Id: Ic43ee09f98ba1e91a0737ab861d9053c368d50b8
diff --git a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
index 88ef3ac..2c0b510 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
@@ -16,6 +16,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static com.google.gerrit.extensions.client.ListChangesOption.ALL_REVISIONS;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_COMMIT;
import static com.google.gerrit.extensions.client.ListChangesOption.CURRENT_REVISION;
@@ -25,6 +26,7 @@
import com.google.common.base.Charsets;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
import com.google.common.io.CharStreams;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.acceptance.LightweightPluginDaemonTest;
@@ -32,6 +34,7 @@
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.accounts.AccountApi;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.RebaseInput;
@@ -56,8 +59,8 @@
import java.io.InputStreamReader;
import java.sql.Timestamp;
import java.util.ArrayList;
-import java.util.EnumSet;
import java.util.List;
+import java.util.Optional;
import java.util.concurrent.TimeUnit;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
@@ -99,20 +102,14 @@
ChangeInfo dsOneChangeInfo = sortedChanges.get(0);
assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
- ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(dsOneChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertAutomergerChangeCreatedMessage(dsOneChangeInfo.id);
ChangeInfo dsTwoChangeInfo = sortedChanges.get(1);
assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
- ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertAutomergerChangeCreatedMessage(dsTwoChangeInfo.id);
ChangeInfo masterChangeInfo = sortedChanges.get(2);
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo(null);
+ assertCodeReview(masterChangeInfo.id, 0, null);
assertThat(masterChangeInfo.branch).isEqualTo("master");
// Ensure that commit subjects are correct
@@ -126,9 +123,9 @@
// +2 and submit
merge(result);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(2);
+ assertCodeReview(masterChangeInfo.id, 2, null);
+ assertCodeReview(dsOneChangeInfo.id, 2, "autogenerated:Automerger");
+ assertCodeReview(dsTwoChangeInfo.id, 2, "autogenerated:Automerger");
}
@Test
@@ -176,33 +173,23 @@
// Should create two changes on bottom, since left and right are different.
ChangeInfo bottomChangeInfoA = sortedChanges.get(0);
assertThat(bottomChangeInfoA.branch).isEqualTo("bottom");
- ChangeApi bottomChangeA = gApi.changes().id(bottomChangeInfoA._number);
- assertThat(getVote(bottomChangeA, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(bottomChangeA, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertCodeReview(bottomChangeInfoA.id, 2, "autogenerated:Automerger");
ChangeInfo bottomChangeInfoB = sortedChanges.get(1);
assertThat(bottomChangeInfoB.branch).isEqualTo("bottom");
- ChangeApi bottomChangeB = gApi.changes().id(bottomChangeInfoB._number);
- assertThat(getVote(bottomChangeB, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(bottomChangeB, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertCodeReview(bottomChangeInfoB.id, 2, "autogenerated:Automerger");
ChangeInfo leftChangeInfo = sortedChanges.get(2);
assertThat(leftChangeInfo.branch).isEqualTo("left");
- ChangeApi leftChange = gApi.changes().id(leftChangeInfo._number);
- assertThat(getVote(leftChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(leftChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertCodeReview(leftChangeInfo.id, 2, "autogenerated:Automerger");
ChangeInfo masterChangeInfo = sortedChanges.get(3);
assertThat(masterChangeInfo.branch).isEqualTo("master");
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo(null);
+ assertCodeReview(masterChangeInfo.id, 2, null);
ChangeInfo rightChangeInfo = sortedChanges.get(4);
assertThat(rightChangeInfo.branch).isEqualTo("right");
- ChangeApi rightChange = gApi.changes().id(rightChangeInfo._number);
- assertThat(getVote(rightChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(rightChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertCodeReview(rightChangeInfo.id, 2, "autogenerated:Automerger");
// Ensure that commit subjects are correct
String masterSubject = masterChangeInfo.subject;
@@ -276,27 +263,19 @@
// Should create two changes on bottom, since left and right are different.
ChangeInfo bottomChangeInfo = sortedChanges.get(0);
assertThat(bottomChangeInfo.branch).isEqualTo("bottom");
- ChangeApi bottomChange = gApi.changes().id(bottomChangeInfo._number);
- assertThat(getVote(bottomChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(bottomChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertCodeReview(bottomChangeInfo.id, 2, "autogenerated:Automerger");
ChangeInfo leftChangeInfo = sortedChanges.get(1);
assertThat(leftChangeInfo.branch).isEqualTo("left");
- ChangeApi leftChange = gApi.changes().id(leftChangeInfo._number);
- assertThat(getVote(leftChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(leftChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertCodeReview(leftChangeInfo.id, 2, "autogenerated:Automerger");
ChangeInfo masterChangeInfo = sortedChanges.get(2);
assertThat(masterChangeInfo.branch).isEqualTo("master");
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo(null);
+ assertCodeReview(masterChangeInfo.id, 2, null);
ChangeInfo rightChangeInfo = sortedChanges.get(3);
assertThat(rightChangeInfo.branch).isEqualTo("right");
- ChangeApi rightChange = gApi.changes().id(rightChangeInfo._number);
- assertThat(getVote(rightChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(rightChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertCodeReview(rightChangeInfo.id, 2, "autogenerated:Automerger");
// Ensure that commit subjects are correct
String masterSubject = masterChangeInfo.subject;
@@ -455,28 +434,24 @@
ChangeInfo dsOneChangeInfo = sortedChanges.get(0);
assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
- ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(dsOneChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertAutomergerChangeCreatedMessage(dsOneChangeInfo.id);
// It should skip ds_one, since this is a DO NOT MERGE
+ ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
assertThat(dsOneChange.get().subject).contains("skipped:");
assertThat(dsOneChange.current().files().keySet()).contains("filename");
assertThat(dsOneChange.current().files().get("filename").linesDeleted).isEqualTo(1);
ChangeInfo dsTwoChangeInfo = sortedChanges.get(1);
assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
- ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertAutomergerChangeCreatedMessage(dsTwoChangeInfo.id);
// It should not skip ds_two, since it is marked with mergeAll: true
+ ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
assertThat(dsTwoChange.get().subject).doesNotContain("skipped:");
BinaryResult dsTwoContent = dsTwoChange.current().file("filename").content();
assertThat(dsTwoContent.asString()).isEqualTo(content.asString());
ChangeInfo masterChangeInfo = sortedChanges.get(2);
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo(null);
+ assertCodeReview(masterChangeInfo.id, 0, null);
assertThat(masterChangeInfo.branch).isEqualTo("master");
// Ensure that commit subjects are correct
@@ -519,9 +494,8 @@
ChangeInfo dsOneChangeInfo = sortedChanges.get(0);
assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
+ assertAutomergerChangeCreatedMessage(dsOneChangeInfo.id);
ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(dsOneChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
// It should skip ds_one, since this is a DO NOT MERGE ANYWHERE
assertThat(dsOneChange.get().subject).contains("skipped:");
assertThat(dsOneChange.current().files().keySet()).contains("filename");
@@ -529,18 +503,15 @@
ChangeInfo dsTwoChangeInfo = sortedChanges.get(1);
assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
+ assertAutomergerChangeCreatedMessage(dsTwoChangeInfo.id);
ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
// It should skip ds_one, since this is a DO NOT MERGE ANYWHERE
assertThat(dsTwoChange.get().subject).contains("skipped:");
assertThat(dsTwoChange.current().files().keySet()).contains("filename");
assertThat(dsTwoChange.current().files().get("filename").linesDeleted).isEqualTo(1);
ChangeInfo masterChangeInfo = sortedChanges.get(2);
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo(null);
+ assertCodeReview(masterChangeInfo.id, 0, null);
assertThat(masterChangeInfo.branch).isEqualTo("master");
// Ensure that commit subjects are correct
@@ -594,15 +565,11 @@
ChangeInfo dsTwoChangeInfo = sortedChanges.get(0);
assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
- ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
// This is -2 because the -2 vote from master propagated to ds_two
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(-2);
- assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertCodeReview(dsTwoChangeInfo.id, -2, "autogenerated:Automerger");
ChangeInfo masterChangeInfo = sortedChanges.get(1);
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(-2);
- assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo("autogenerated:MergeConflict");
+ assertCodeReview(masterChangeInfo.id, -2, "autogenerated:MergeConflict");
assertThat(masterChangeInfo.branch).isEqualTo("master");
// Make sure that merge conflict message is still added
@@ -665,16 +632,13 @@
ChangeInfo dsTwoChangeInfo = sortedChanges.get(0);
assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
- ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(dsTwoChange, "Code-Review").tag).isEqualTo("autogenerated:Automerger");
+ assertAutomergerChangeCreatedMessage(dsTwoChangeInfo.id);
ChangeInfo masterChangeInfo = sortedChanges.get(1);
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
// This is 0 because the -2 vote on master failed due to permissions
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
- assertThat(getVote(masterChange, "Code-Review").tag).isEqualTo("autogenerated:MergeConflict");
+ assertCodeReview(masterChangeInfo.id, 0, null);
assertThat(masterChangeInfo.branch).isEqualTo("master");
+ assertThat(getLastMessage(masterChangeInfo.id).tag).isEqualTo("autogenerated:MergeConflict");
// Make sure that merge conflict message is still added
List<String> messages = new ArrayList<>();
@@ -872,17 +836,15 @@
// Check that downstream is at Code-Review 0
ChangeInfo dsOneChangeInfo = sortedChanges.get(0);
assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
- ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(0);
+ assertCodeReview(dsOneChangeInfo.id, 0, null);
// Try to +2 master and see it succeed to +2 master and ds_one
ChangeInfo masterChangeInfo = sortedChanges.get(2);
assertThat(masterChangeInfo.branch).isEqualTo("master");
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
+ assertCodeReviewMissing(masterChangeInfo.id);
approve(masterChangeInfo.id);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(2);
+ assertCodeReview(masterChangeInfo.id, 2, null);
+ assertCodeReview(dsOneChangeInfo.id, 2, "autogenerated:Automerger");
// Try to +2 downstream and see it fail
exception.expect(AuthException.class);
@@ -941,31 +903,28 @@
// Check that downstream is at Code-Review 0
ChangeInfo dsOneChangeInfo = sortedChanges.get(0);
assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
- ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(0);
+ assertCodeReview(dsOneChangeInfo.id, 0, null);
// Try to +1 master and see it succeed to +1 master and ds_one
ChangeInfo masterChangeInfo = sortedChanges.get(2);
assertThat(masterChangeInfo.branch).isEqualTo("master");
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
+ assertCodeReviewMissing(masterChangeInfo.id);
recommend(masterChangeInfo.id);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(1);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(1);
+ assertCodeReview(masterChangeInfo.id, 1, null);
+ assertCodeReview(dsOneChangeInfo.id, 1, "autogenerated:Automerger");
ChangeInfo dsTwoChangeInfo = sortedChanges.get(1);
assertThat(dsTwoChangeInfo.branch).isEqualTo("ds_two");
- ChangeApi dsTwoChange = gApi.changes().id(dsTwoChangeInfo._number);
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(1);
+ assertCodeReview(dsTwoChangeInfo.id, 1, "autogenerated:Automerger");
// +2 ds_one and see that it overrides the +1 of the contextUser
approve(dsOneChangeInfo.id);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(2);
+ assertCodeReview(dsOneChangeInfo.id, 2, null);
+ assertCodeReview(dsTwoChangeInfo.id, 2, "autogenerated:Automerger");
// +0 ds_one and see that it goes back to the +1 of the contextUser
gApi.changes().id(dsOneChangeInfo.id).revision("current").review(ReviewInput.noScore());
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(1);
- assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(1);
+ assertCodeReview(dsOneChangeInfo.id, 1, "autogenerated:Automerger");
+ assertCodeReview(dsTwoChangeInfo.id, 1, "autogenerated:Automerger");
}
@Test
@@ -1037,8 +996,7 @@
// Check that master is at Code-Review -2
ChangeInfo masterChangeInfo = sortedChanges.get(1);
assertThat(masterChangeInfo.branch).isEqualTo("master");
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(-2);
+ assertCodeReview(masterChangeInfo.id, -2, "autogenerated:MergeConflict");
}
private Project.NameKey defaultSetup() throws Exception {
@@ -1122,17 +1080,37 @@
pushConfig(options, "context_user.config");
}
- private ApprovalInfo getVote(ChangeApi change, String label) throws RestApiException {
- List<ApprovalInfo> approvals = change.get(EnumSet.of(DETAILED_LABELS)).labels.get(label).all;
- ApprovalInfo maxApproval = null;
- for (ApprovalInfo approval : approvals) {
- if (maxApproval == null) {
- maxApproval = approval;
- } else if (approval.value != null && approval.value > maxApproval.value) {
- maxApproval = approval;
- }
+ private Optional<ApprovalInfo> getCodeReview(String id) throws RestApiException {
+ List<ApprovalInfo> approvals =
+ gApi.changes().id(id).get(DETAILED_LABELS).labels.get("Code-Review").all;
+ if (approvals == null) {
+ return Optional.empty();
}
- return maxApproval;
+ return approvals.stream().max(comparing(a -> a.value));
+ }
+
+ private void assertCodeReview(String id, int expectedValue, @Nullable String expectedTag)
+ throws RestApiException {
+ Optional<ApprovalInfo> vote = getCodeReview(id);
+ assertThat(vote).named("Code-Review vote").isPresent();
+ assertThat(vote.get().value).named("value of Code-Review vote").isEqualTo(expectedValue);
+ assertThat(vote.get().tag).named("tag of Code-Review vote").isEqualTo(expectedTag);
+ }
+
+ private void assertCodeReviewMissing(String id) throws RestApiException {
+ assertThat(getCodeReview(id)).isEmpty();
+ }
+
+ private void assertAutomergerChangeCreatedMessage(String id) throws RestApiException {
+ ChangeMessageInfo message = getLastMessage(id);
+ assertThat(message.message).contains("Automerger change created!");
+ assertThat(message.tag).isEqualTo("autogenerated:Automerger");
+ }
+
+ private ChangeMessageInfo getLastMessage(String id) throws RestApiException {
+ List<ChangeMessageInfo> messages = gApi.changes().id(id).messages();
+ assertThat(messages).isNotEmpty();
+ return Iterables.getLast(messages);
}
private ImmutableList<ChangeInfo> sortedChanges(List<ChangeInfo> changes) {
