From cd5ede6c7ee4aa510a2b84618960c082136f6a56 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Sat, 10 Nov 2018 20:45:25 +0100 Subject: [PATCH 1/3] Use new test API for group creation in AbstractDaemonTest Change-Id: Id9c8c438413557852e237ebe99f7f7396dbd8d9e --- .../gerrit/acceptance/AbstractDaemonTest.java | 21 +++---------------- .../acceptance/api/change/ChangeIT.java | 4 ++-- .../api/group/GroupsConsistencyIT.java | 2 +- .../gerrit/acceptance/api/group/GroupsIT.java | 20 ++++++++++++++++++ 4 files changed, 26 insertions(+), 21 deletions(-) diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index d168919169..17e6bf6a11 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -38,6 +38,7 @@ import com.google.common.jimfs.Jimfs; import com.google.common.primitives.Chars; import com.google.gerrit.acceptance.AcceptanceTestRequestScope.Context; import com.google.gerrit.acceptance.testsuite.account.TestSshKeys; +import com.google.gerrit.acceptance.testsuite.group.GroupOperations; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.ContributorAgreement; @@ -55,7 +56,6 @@ import com.google.gerrit.extensions.api.changes.ReviewInput; import com.google.gerrit.extensions.api.changes.RevisionApi; import com.google.gerrit.extensions.api.changes.SubmittedTogetherInfo; import com.google.gerrit.extensions.api.groups.GroupApi; -import com.google.gerrit.extensions.api.groups.GroupInput; import com.google.gerrit.extensions.api.projects.BranchApi; import com.google.gerrit.extensions.api.projects.BranchInput; import com.google.gerrit.extensions.api.projects.ProjectInput; @@ -264,6 +264,7 @@ public abstract class AbstractDaemonTest { @Inject protected ChangeNotes.Factory notesFactory; @Inject protected BatchAbandon batchAbandon; @Inject protected TestSshKeys sshKeys; + @Inject protected GroupOperations groupOperations; protected EventRecorder eventRecorder; protected GerritServer server; @@ -1194,23 +1195,7 @@ public abstract class AbstractDaemonTest { } protected String createGroup(String name) throws Exception { - return createGroup(name, "Administrators"); - } - - protected String createGroupWithRealName(String name) throws Exception { - GroupInput in = new GroupInput(); - in.name = name; - in.ownerId = "Administrators"; - gApi.groups().create(in); - return name; - } - - protected String createGroup(String name, String owner) throws Exception { - name = name(name); - GroupInput in = new GroupInput(); - in.name = name; - in.ownerId = owner; - gApi.groups().create(in); + groupOperations.newGroup().name(name).create(); return name; } diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 30aef735ab..2941d8cc09 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -1823,7 +1823,7 @@ public class ChangeIT extends AbstractDaemonTest { .preferredEmail(email) .fullname(fullname) .create(); - String testGroup = createGroupWithRealName("ab"); + String testGroup = createGroup("ab"); GroupApi groupApi = gApi.groups().id(testGroup); groupApi.description("test group"); groupApi.addMembers(user.fullName); @@ -1884,7 +1884,7 @@ public class ChangeIT extends AbstractDaemonTest { .fullname(myGroupUserFullname) .create(); - String testGroup = createGroupWithRealName("kobe"); + String testGroup = createGroup("kobe"); GroupApi groupApi = gApi.groups().id(testGroup); groupApi.description("test group"); groupApi.addMembers(myGroupUserFullname); diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java index 87a566e237..f69e356cb6 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java @@ -218,7 +218,7 @@ public class GroupsConsistencyIT extends AbstractDaemonTest { @Test public void cyclicSubgroup() throws Exception { updateGroupFile(RefNames.refsGroups(new AccountGroup.UUID(g1.id)), "subgroups", g1.id + "\n"); - assertWarning("cyclic"); + assertWarning("cycle"); } private void assertError(String msg) throws Exception { diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index ade0f3c144..c31fd6dc58 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -144,6 +144,26 @@ public class GroupsIT extends AbstractDaemonTest { } } + // Creates a group, but with uniquified name. + @Override + protected String createGroup(String name) throws Exception { + name = name(name); + GroupInput in = new GroupInput(); + in.name = name; + in.ownerId = "Administrators"; + gApi.groups().create(in); + return name; + } + + protected String createGroup(String name, String owner) throws Exception { + name = name(name); + GroupInput in = new GroupInput(); + in.name = name; + in.ownerId = owner; + gApi.groups().create(in); + return name; + } + @Override protected ProjectResetter.Config resetProjects() { // Don't reset All-Users since deleting users makes groups inconsistent (e.g. groups would From a9ae62dc15f23d8b52fc7260db95c4f0065a9140 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Sat, 10 Nov 2018 20:45:25 +0100 Subject: [PATCH 2/3] Push group operations down into leaf test classes. Change-Id: I3d1bf71764a623f9bbbed53fe1d0db4a50328142 --- .../gerrit/acceptance/AbstractDaemonTest.java | 8 ++----- .../acceptance/api/accounts/AccountIT.java | 8 ++++++- .../acceptance/api/change/ChangeIT.java | 7 +++++-- .../api/group/GroupsConsistencyIT.java | 8 +++++-- .../gerrit/acceptance/api/group/GroupsIT.java | 11 ++++------ .../rest/change/ChangeReviewersIT.java | 21 ++++++++++++------- .../acceptance/rest/change/IndexChangeIT.java | 8 ++++++- 7 files changed, 44 insertions(+), 27 deletions(-) diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index 17e6bf6a11..e523e84c66 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -1194,11 +1194,6 @@ public abstract class AbstractDaemonTest { return changeResourceFactory.create(notes.get(0), atrScope.get().getUser()); } - protected String createGroup(String name) throws Exception { - groupOperations.newGroup().name(name).create(); - return name; - } - protected RevCommit getHead(Repository repo, String name) throws Exception { try (RevWalk rw = new RevWalk(repo)) { Ref r = repo.exactRef(name); @@ -1233,7 +1228,8 @@ public abstract class AbstractDaemonTest { protected ContributorAgreement configureContributorAgreement(boolean autoVerify) throws Exception { ContributorAgreement ca; - String g = createGroup(autoVerify ? "cla-test-group" : "cla-test-no-auto-verify-group"); + String name = autoVerify ? "cla-test-group" : "cla-test-no-auto-verify-group"; + String g = groupOperations.newGroup().name(name).create().get(); GroupApi groupApi = gApi.groups().id(g); groupApi.description("CLA test group"); InternalGroup caGroup = group(new AccountGroup.UUID(groupApi.detail().id)); diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java index 5e46a03126..9886db53a2 100644 --- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java +++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java @@ -56,6 +56,7 @@ import com.google.gerrit.acceptance.TestAccount; import com.google.gerrit.acceptance.UseSsh; import com.google.gerrit.acceptance.testsuite.account.AccountOperations; import com.google.gerrit.acceptance.testsuite.account.TestSshKeys; +import com.google.gerrit.acceptance.testsuite.group.GroupOperations; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GlobalCapability; @@ -97,6 +98,7 @@ import com.google.gerrit.gpg.PublicKeyStore; import com.google.gerrit.gpg.testing.TestKey; import com.google.gerrit.mail.Address; import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; @@ -233,6 +235,8 @@ public class AccountIT extends AbstractDaemonTest { @Inject private AccountManager accountManager; + @Inject protected GroupOperations groupOperations; + private AccountIndexedCounter accountIndexedCounter; private RegistrationHandle accountIndexEventCounterHandle; private RefUpdateCounter refUpdateCounter; @@ -2208,7 +2212,9 @@ public class AccountIT extends AbstractDaemonTest { public void allGroupsForAUserAccountCanBeRetrieved() throws Exception { String username = name("user1"); accountOperations.newAccount().username(username).create(); - String group = createGroup("group"); + AccountGroup.UUID groupID = groupOperations.newGroup().name("group").create(); + String group = groupOperations.group(groupID).get().name(); + gApi.groups().id(group).addMembers(username); List allGroups = gApi.accounts().id(username).getGroups(); diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 2941d8cc09..744d1e9661 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -68,6 +68,7 @@ import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestProjectInput; import com.google.gerrit.acceptance.testsuite.account.AccountOperations; +import com.google.gerrit.acceptance.testsuite.group.GroupOperations; import com.google.gerrit.common.FooterConstants; import com.google.gerrit.common.data.LabelFunction; import com.google.gerrit.common.data.LabelType; @@ -194,6 +195,8 @@ public class ChangeIT extends AbstractDaemonTest { @Inject private ChangeIndexCollection changeIndexCollection; @Inject private IndexConfig indexConfig; + @Inject protected GroupOperations groupOperations; + private ChangeIndexedCounter changeIndexedCounter; private RegistrationHandle changeIndexedCounterHandle; @@ -1823,7 +1826,7 @@ public class ChangeIT extends AbstractDaemonTest { .preferredEmail(email) .fullname(fullname) .create(); - String testGroup = createGroup("ab"); + String testGroup = groupOperations.newGroup().name("ab").create().get(); GroupApi groupApi = gApi.groups().id(testGroup); groupApi.description("test group"); groupApi.addMembers(user.fullName); @@ -1884,7 +1887,7 @@ public class ChangeIT extends AbstractDaemonTest { .fullname(myGroupUserFullname) .create(); - String testGroup = createGroup("kobe"); + String testGroup = groupOperations.newGroup().name("kobe").create().get(); GroupApi groupApi = gApi.groups().id(testGroup); groupApi.description("test group"); groupApi.addMembers(myGroupUserFullname); diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java index f69e356cb6..491cb3a300 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsConsistencyIT.java @@ -20,6 +20,7 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.Sandboxed; +import com.google.gerrit.acceptance.testsuite.group.GroupOperations; import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo; import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; @@ -30,6 +31,7 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.group.db.GroupConfig; import com.google.gerrit.server.group.db.GroupNameNotes; import com.google.gerrit.server.group.db.testing.GroupTestUtil; +import com.google.inject.Inject; import java.util.List; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.RefRename; @@ -47,6 +49,8 @@ import org.junit.Test; @Sandboxed @NoHttpd public class GroupsConsistencyIT extends AbstractDaemonTest { + + @Inject protected GroupOperations groupOperations; private GroupInfo gAdmin; private GroupInfo g1; private GroupInfo g2; @@ -57,8 +61,8 @@ public class GroupsConsistencyIT extends AbstractDaemonTest { public void basicSetup() throws Exception { allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); - String name1 = createGroup("g1"); - String name2 = createGroup("g2"); + String name1 = groupOperations.newGroup().name("g1").create().get(); + String name2 = groupOperations.newGroup().name("g2").create().get(); gApi.groups().id(name1).addMembers(user.fullName); gApi.groups().id(name2).addMembers(admin.fullName); diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index c31fd6dc58..4a4ee2ac93 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -145,14 +145,11 @@ public class GroupsIT extends AbstractDaemonTest { } // Creates a group, but with uniquified name. - @Override protected String createGroup(String name) throws Exception { - name = name(name); - GroupInput in = new GroupInput(); - in.name = name; - in.ownerId = "Administrators"; - gApi.groups().create(in); - return name; + // TODO(hanwen): rewrite this test in terms of UUID. This requires redoing the assertion helpers + // too. + AccountGroup.UUID g = groupOperations.newGroup().ownerGroupUuid(adminGroupUuid()).create(); + return groupRef(g).getName(); } protected String createGroup(String name, String owner) throws Exception { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java index d1f4d8420f..6a9a27c895 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java @@ -31,6 +31,7 @@ import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.acceptance.Sandboxed; import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.testsuite.group.GroupOperations; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.changes.AddReviewerInput; import com.google.gerrit.extensions.api.changes.AddReviewerResult; @@ -52,6 +53,7 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.change.ReviewerAdder; import com.google.gerrit.testing.FakeEmailSender.Message; import com.google.gson.stream.JsonReader; +import com.google.inject.Inject; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -62,12 +64,15 @@ import java.util.Map; import org.junit.Test; public class ChangeReviewersIT extends AbstractDaemonTest { + + @Inject protected GroupOperations groupOperations; + @Test public void addGroupAsReviewer() throws Exception { // Set up two groups, one that is too large too add as reviewer, and one // that is too large to add without confirmation. - String largeGroup = createGroup("largeGroup"); - String mediumGroup = createGroup("mediumGroup"); + String largeGroup = groupOperations.newGroup().name("largeGroup").create().get(); + String mediumGroup = groupOperations.newGroup().name("mediumGroup").create().get(); int largeGroupSize = ReviewerAdder.DEFAULT_MAX_REVIEWERS + 1; int mediumGroupSize = ReviewerAdder.DEFAULT_MAX_REVIEWERS_WITHOUT_CHECK + 1; @@ -170,7 +175,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest { PushOneCommit.Result r = createChange(); String changeId = r.getChangeId(); AddReviewerInput in = new AddReviewerInput(); - in.reviewer = createGroup("cc1"); + in.reviewer = groupOperations.newGroup().name("cc1").create().get(); in.state = CC; gApi.groups() .id(in.reviewer) @@ -209,7 +214,7 @@ public class ChangeReviewersIT extends AbstractDaemonTest { result = addReviewer(changeId, reviewer.username); assertThat(result.error).isNull(); sender.clear(); - in.reviewer = createGroup("cc2"); + in.reviewer = groupOperations.newGroup().name("cc2").create().get(); gApi.groups().id(in.reviewer).addMembers(usernames.toArray(new String[usernames.size()])); gApi.groups().id(in.reviewer).addMembers(reviewer.username); result = addReviewer(changeId, in); @@ -479,8 +484,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest { usernames.add(u.username); } - String largeGroup = createGroup("largeGroup"); - String mediumGroup = createGroup("mediumGroup"); + String largeGroup = groupOperations.newGroup().name("largeGroup").create().get(); + String mediumGroup = groupOperations.newGroup().name("mediumGroup").create().get(); gApi.groups().id(largeGroup).addMembers(usernames.toArray(new String[largeGroupSize])); gApi.groups() .id(mediumGroup) @@ -622,8 +627,8 @@ public class ChangeReviewersIT extends AbstractDaemonTest { accountCreator.create(name("user2"), emailPrefix + "user2@example.com", "User2"); TestAccount user3 = accountCreator.create(name("user3"), emailPrefix + "user3@example.com", "User3"); - String group1 = createGroup("group1"); - String group2 = createGroup("group2"); + String group1 = groupOperations.newGroup().name("group1").create().get(); + String group2 = groupOperations.newGroup().name("group2").create().get(); gApi.groups().id(group1).addMembers(user1.username, user2.username); gApi.groups().id(group2).addMembers(user2.username, user3.username); diff --git a/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java index 6555fe8307..bb114e70ca 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/IndexChangeIT.java @@ -20,17 +20,22 @@ import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.testsuite.group.GroupOperations; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.project.testing.Util; +import com.google.inject.Inject; import java.util.List; import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository; import org.eclipse.jgit.junit.TestRepository; import org.junit.Test; public class IndexChangeIT extends AbstractDaemonTest { + + @Inject protected GroupOperations groupOperations; + @Test public void indexChange() throws Exception { String changeId = createChange().getChangeId(); @@ -48,7 +53,8 @@ public class IndexChangeIT extends AbstractDaemonTest { public void indexChangeAfterOwnerLosesVisibility() throws Exception { // Create a test group with 2 users as members TestAccount user2 = accountCreator.user2(); - String group = createGroup("test"); + AccountGroup.UUID groupId = groupOperations.newGroup().name("test").create(); + String group = groupOperations.group(groupId).get().name(); gApi.groups().id(group).addMembers("admin", "user", user2.username); // Create a project and restrict its visibility to the group From 3d03743421110ce542b8cc732517b6d10715b127 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Sat, 10 Nov 2018 15:04:56 +0100 Subject: [PATCH 3/3] Mark getRemoteHead as @Nullable Change-Id: I338df11c6ce752f9313b7945ab028cf0797eed69 --- java/com/google/gerrit/acceptance/AbstractDaemonTest.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java index e523e84c66..0107bae598 100644 --- a/java/com/google/gerrit/acceptance/AbstractDaemonTest.java +++ b/java/com/google/gerrit/acceptance/AbstractDaemonTest.java @@ -1194,6 +1194,7 @@ public abstract class AbstractDaemonTest { return changeResourceFactory.create(notes.get(0), atrScope.get().getUser()); } + @Nullable protected RevCommit getHead(Repository repo, String name) throws Exception { try (RevWalk rw = new RevWalk(repo)) { Ref r = repo.exactRef(name); @@ -1205,16 +1206,19 @@ public abstract class AbstractDaemonTest { return getHead(repo, "HEAD"); } + @Nullable protected RevCommit getRemoteHead(Project.NameKey project, String branch) throws Exception { try (Repository repo = repoManager.openRepository(project)) { return getHead(repo, branch.startsWith(Constants.R_REFS) ? branch : "refs/heads/" + branch); } } + @Nullable protected RevCommit getRemoteHead(String project, String branch) throws Exception { return getRemoteHead(new Project.NameKey(project), branch); } + @Nullable protected RevCommit getRemoteHead() throws Exception { return getRemoteHead(project, "master"); }