Migrate SuggestReviewersIT completely to GroupOperations
The logic for reviewer suggestion only includes groups which have at
least one member which can see the specified change. This automatically
means that groups without members don't see any changes and never show
up in any suggestions. However, some of the tests assumed that group3
(without any explicitly specified members) could still see a created
change. This only worked as the admin user was automatically added. The
new test API for groups doesn't automatically add the admin user. Hence,
we now add another user so that group3 can still see created changes.
Change-Id: If6ad8ebf43e9e1ebbd4965712a5466321094e013
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
index be05d6f..8656256 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/SuggestReviewersIT.java
@@ -31,16 +31,12 @@
import com.google.gerrit.extensions.api.accounts.EmailInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeInput;
-import com.google.gerrit.extensions.common.GroupInfo;
import com.google.gerrit.extensions.common.SuggestedReviewerInfo;
-import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.RestApiException;
-import com.google.gerrit.extensions.restapi.TopLevelResource;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.group.InternalGroup;
-import com.google.gerrit.server.restapi.group.CreateGroup;
import com.google.inject.Inject;
import java.util.Arrays;
import java.util.List;
@@ -50,14 +46,13 @@
import org.junit.Test;
public class SuggestReviewersIT extends AbstractDaemonTest {
- @Inject private CreateGroup createGroup;
@Inject private ProjectOperations projectOperations;
@Inject private AccountOperations accountOperations;
@Inject private GroupOperations groupOperations;
- private InternalGroup group1;
- private InternalGroup group2;
- private InternalGroup group3;
+ private AccountGroup.UUID group1;
+ private AccountGroup.UUID group2;
+ private AccountGroup.UUID group3;
private TestAccount user1;
private TestAccount user2;
@@ -66,14 +61,14 @@
@Before
public void setUp() throws Exception {
- group1 = newGroup("users1");
- group2 = newGroup("users2");
- group3 = newGroup("users3");
-
- user1 = user("user1", "First1 Last1", group1);
- user2 = user("user2", "First2 Last2", group2);
- user3 = user("user3", "First3 Last3", group1, group2);
+ user1 = user("user1", "First1 Last1");
+ user2 = user("user2", "First2 Last2");
+ user3 = user("user3", "First3 Last3");
user4 = user("jdoe", "John Doe", "JDOE");
+
+ group1 = groupOperations.newGroup().name(name("users1")).members(user1.id, user3.id).create();
+ group2 = groupOperations.newGroup().name(name("users2")).members(user2.id, user3.id).create();
+ group3 = groupOperations.newGroup().name(name("users3")).members(user1.id).create();
}
@Test
@@ -115,7 +110,8 @@
assertReviewers(
reviewers, ImmutableList.of(user1, user2, user3), ImmutableList.of(group1, group2));
- reviewers = suggestReviewers(changeId, group3.getName(), 10);
+ String group3Name = groupOperations.group(group3).get().name();
+ reviewers = suggestReviewers(changeId, group3Name, 10);
assertReviewers(reviewers, ImmutableList.of(), ImmutableList.of(group3));
// Suggested accounts are ordered by activity. All users have no activity,
@@ -161,7 +157,7 @@
setApiUser(user3);
block("refs/*", "read", ANONYMOUS_USERS);
- allow("refs/*", "read", group1.getGroupUUID());
+ allow("refs/*", "read", group1);
reviewers = suggestReviewers(changeId, user2.username, 2);
assertThat(reviewers).isEmpty();
}
@@ -177,7 +173,7 @@
assertThat(reviewers).isEmpty();
setApiUser(user1); // Clear cached group info.
- allowGlobalCapabilities(group1.getGroupUUID(), GlobalCapability.VIEW_ALL_ACCOUNTS);
+ allowGlobalCapabilities(group1, GlobalCapability.VIEW_ALL_ACCOUNTS);
reviewers = suggestReviewers(changeId, user2.username, 2);
assertThat(reviewers).hasSize(1);
assertThat(Iterables.getOnlyElement(reviewers).account.name).isEqualTo(user2.fullName);
@@ -253,17 +249,11 @@
}
@Test
- @GerritConfig(name = "addreviewer.maxAllowed", value = "2")
+ @GerritConfig(name = "addreviewer.maxAllowed", value = "1")
@GerritConfig(name = "addreviewer.maxWithoutConfirmation", value = "1")
- public void suggestReviewersGroupSizeConsiderations() throws Exception {
- InternalGroup largeGroup = newGroup("large");
- InternalGroup mediumGroup = newGroup("medium");
-
- // Both groups have Administrator as a member. Add two users to large
- // group to push it past maxAllowed, and one to medium group to push it
- // past maxWithoutConfirmation.
- user("individual 0", "Test0 Last0", largeGroup, mediumGroup);
- user("individual 1", "Test1 Last1", largeGroup);
+ public void confirmationIsNeverRequestedForAccounts() throws Exception {
+ user("individual 0", "Test0 Last0");
+ user("individual 1", "Test1 Last1");
String changeId = createChange().getChangeId();
List<SuggestedReviewerInfo> reviewers;
@@ -275,16 +265,30 @@
reviewer = reviewers.get(0);
assertThat(reviewer.count).isEqualTo(1);
assertThat(reviewer.confirm).isNull();
+ }
+
+ @Test
+ @GerritConfig(name = "addreviewer.maxAllowed", value = "2")
+ @GerritConfig(name = "addreviewer.maxWithoutConfirmation", value = "1")
+ public void suggestReviewersGroupSizeConsiderations() throws Exception {
+ AccountGroup.UUID largeGroup = createGroupWithArbitraryMembers(3);
+ String largeGroupName = groupOperations.group(largeGroup).get().name();
+ AccountGroup.UUID mediumGroup = createGroupWithArbitraryMembers(2);
+ String mediumGroupName = groupOperations.group(mediumGroup).get().name();
+
+ String changeId = createChange().getChangeId();
+ List<SuggestedReviewerInfo> reviewers;
+ SuggestedReviewerInfo reviewer;
// Large group should never be suggested.
- reviewers = suggestReviewers(changeId, largeGroup.getName(), 10);
+ reviewers = suggestReviewers(changeId, largeGroupName, 10);
assertThat(reviewers).isEmpty();
// Medium group should be suggested with appropriate count and confirm.
- reviewers = suggestReviewers(changeId, mediumGroup.getName(), 10);
+ reviewers = suggestReviewers(changeId, mediumGroupName, 10);
assertThat(reviewers).hasSize(1);
reviewer = reviewers.get(0);
- assertThat(reviewer.group.name).isEqualTo(mediumGroup.getName());
+ assertThat(reviewer.group.id).isEqualTo(mediumGroup.get());
assertThat(reviewer.count).isEqualTo(2);
assertThat(reviewer.confirm).isTrue();
}
@@ -541,12 +545,6 @@
return groupOperations.newGroup().members(members).create();
}
- private InternalGroup newGroup(String name) throws Exception {
- GroupInfo group =
- createGroup.apply(TopLevelResource.INSTANCE, IdString.fromDecoded(name(name)), null);
- return group(new AccountGroup.UUID(group.id));
- }
-
private TestAccount user(String name, String fullName, String emailName, InternalGroup... groups)
throws Exception {
String[] groupNames = Arrays.stream(groups).map(InternalGroup::getName).toArray(String[]::new);
@@ -576,10 +574,10 @@
return gApi.changes().create(ci).get().changeId;
}
- private void assertReviewers(
+ private static void assertReviewers(
List<SuggestedReviewerInfo> actual,
List<TestAccount> expectedUsers,
- List<InternalGroup> expectedGroups) {
+ List<AccountGroup.UUID> expectedGroups) {
List<Integer> actualAccountIds =
actual
.stream()
@@ -593,7 +591,7 @@
actual.stream().filter(i -> i.group != null).map(i -> i.group.id).collect(toList());
assertThat(actualGroupIds)
.containsExactlyElementsIn(
- expectedGroups.stream().map(g -> g.getGroupUUID().get()).collect(toList()))
+ expectedGroups.stream().map(AccountGroup.UUID::get).collect(toList()))
.inOrder();
}
}
