opendev/gerrit
Code Issues Proposed changes

Remove unnecessary checks from AuditLogReaderTest

Browse Source 
The tests which were added to GroupConfigTest in Id3810e02fa cover all
relevant cases for the commit message. There's no need to have tests for
how GroupConfig writes the commit message in AuditLogReaderTest.
Instead, this test class only needs to ensure that AuditLogReader can
parse the commits created by GroupConfig successfully and correctly.

As we already touch AuditLogReaderTest, clean up some small style issues
in addition.

Change-Id: Ib36fc68ebb0e563731a9390c666e7c3305c83a8c
This commit is contained in:
Alice Kober-Sotzek
2017-12-13 18:10:09 +01:00
parent 750843fb30
commit 0612a7c19c
2 changed files with 12 additions and 128 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
javatests/com/google/gerrit/server/group/db/AbstractGroupTest.java
View File

@@ -26,7 +26,6 @@ import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.AllUsersNameProvider;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.CommitUtil;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.testing.GerritBaseTests;
import com.google.gerrit.testing.InMemoryRepositoryManager;
@@ -83,19 +82,6 @@ public class AbstractGroupTest extends GerritBaseTests {
}
}
protected void assertTipCommit(
AccountGroup.UUID uuid, String expectedMessage, String expectedName, String expectedEmail)
throws Exception {
try (RevWalk rw = new RevWalk(allUsersRepo)) {
Ref ref = allUsersRepo.exactRef(RefNames.refsGroups(uuid));
assertCommit(
CommitUtil.toCommitInfo(rw.parseCommit(ref.getObjectId()), rw),
expectedMessage,
expectedName,
expectedEmail);
}
}
protected static void assertServerCommit(CommitInfo commitInfo, String expectedMessage) {
assertCommit(commitInfo, expectedMessage, SERVER_NAME, SERVER_EMAIL);
}

126
javatests/com/google/gerrit/server/group/db/AuditLogReaderTest.java
View File

@@ -23,12 +23,10 @@ import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.AccountGroupByIdAud;
import com.google.gerrit.reviewdb.client.AccountGroupMemberAudit;
import com.google.gerrit.server.account.GroupUUID;
import com.google.gerrit.server.git.CommitUtil;
import com.google.gerrit.server.group.InternalGroup;
import java.sql.Timestamp;
import java.util.Set;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
import org.junit.Before;
import org.junit.Test;
@@ -73,22 +71,12 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
AccountGroupMemberAudit expAudit2 =
createExpMemberAudit(group.getId(), id, userId, getTipTimestamp(uuid));
assertTipCommit(
uuid,
"Update group\n\nAdd: Account 100002 <100002@server-id>",
"Account 100001",
"100001@server-id");
assertThat(auditLogReader.getMembersAudit(allUsersRepo, uuid))
.containsExactly(expAudit1, expAudit2)
.inOrder();
// User removes account 100002 from the group.
removeMembers(uuid, ImmutableSet.of(id));
assertTipCommit(
uuid,
"Update group\n\nRemove: Account 100002 <100002@server-id>",
"Account 100001",
"100001@server-id");
expAudit2.removed(userId, getTipTimestamp(uuid));
assertThat(auditLogReader.getMembersAudit(allUsersRepo, uuid))
@@ -115,14 +103,6 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
AccountGroupMemberAudit expAudit3 =
createExpMemberAudit(groupId, id2, userId, getTipTimestamp(uuid));
assertTipCommit(
uuid,
"Update group\n"
+ "\n"
+ "Add: Account 100002 <100002@server-id>\n"
+ "Add: Account 100003 <100003@server-id>",
"Account 100001",
"100001@server-id");
assertThat(auditLogReader.getMembersAudit(allUsersRepo, uuid))
.containsExactly(expAudit1, expAudit2, expAudit3)
.inOrder();
@@ -137,22 +117,12 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
AccountGroup.UUID subgroupUuid = subgroup.getGroupUUID();
addSubgroups(uuid, ImmutableSet.of(subgroupUuid));
assertTipCommit(
uuid,
String.format("Update group\n\nAdd-group: Group <%s>", subgroupUuid),
"Account 100001",
"100001@server-id");
AccountGroupByIdAud expAudit =
createExpGroupAudit(group.getId(), subgroupUuid, userId, getTipTimestamp(uuid));
assertThat(auditLogReader.getSubgroupsAudit(allUsersRepo, uuid)).containsExactly(expAudit);
removeSubgroups(uuid, ImmutableSet.of(subgroupUuid));
assertTipCommit(
uuid,
String.format("Update group\n\nRemove-group: Group <%s>", subgroupUuid),
"Account 100001",
"100001@server-id");
expAudit.removed(userId, getTipTimestamp(uuid));
assertThat(auditLogReader.getSubgroupsAudit(allUsersRepo, uuid)).containsExactly(expAudit);
@@ -170,15 +140,6 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
addSubgroups(uuid, ImmutableSet.of(subgroupUuid1, subgroupUuid2));
assertTipCommit(
uuid,
"Update group\n"
+ "\n"
+ String.format("Add-group: Group <%s>\n", subgroupUuid1)
+ String.format("Add-group: Group <%s>", subgroupUuid2),
"Account 100001",
"100001@server-id");
AccountGroupByIdAud expAudit1 =
createExpGroupAudit(group.getId(), subgroupUuid1, userId, getTipTimestamp(uuid));
AccountGroupByIdAud expAudit2 =
@@ -209,14 +170,6 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
// Add two accounts.
addMembers(uuid, ImmutableSet.of(id1, id2));
assertTipCommit(
uuid,
"Update group\n"
+ "\n"
+ String.format("Add: Account %s <%s@server-id>\n", id1, id1)
+ String.format("Add: Account %s <%s@server-id>", id2, id2),
"Account 100001",
"100001@server-id");
AccountGroupMemberAudit expMemberAudit1 =
createExpMemberAudit(groupId, id1, userId, getTipTimestamp(uuid));
AccountGroupMemberAudit expMemberAudit2 =
@@ -227,11 +180,6 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
// Add one subgroup.
addSubgroups(uuid, ImmutableSet.of(subgroupUuid1));
assertTipCommit(
uuid,
String.format("Update group\n\nAdd-group: Group <%s>", subgroupUuid1),
"Account 100001",
"100001@server-id");
AccountGroupByIdAud expGroupAudit1 =
createExpGroupAudit(group.getId(), subgroupUuid1, userId, getTipTimestamp(uuid));
assertThat(auditLogReader.getSubgroupsAudit(allUsersRepo, uuid))
@@ -239,11 +187,6 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
// Remove one account.
removeMembers(uuid, ImmutableSet.of(id2));
assertTipCommit(
uuid,
String.format("Update group\n\nRemove: Account %s <%s@server-id>", id2, id2),
"Account 100001",
"100001@server-id");
expMemberAudit2.removed(userId, getTipTimestamp(uuid));
assertThat(auditLogReader.getMembersAudit(allUsersRepo, uuid))
.containsExactly(expMemberAudit, expMemberAudit1, expMemberAudit2)
@@ -251,14 +194,6 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
// Add two subgroups.
addSubgroups(uuid, ImmutableSet.of(subgroupUuid2, subgroupUuid3));
assertTipCommit(
uuid,
"Update group\n"
+ "\n"
+ String.format("Add-group: Group <%s>\n", subgroupUuid2)
+ String.format("Add-group: Group <%s>", subgroupUuid3),
"Account 100001",
"100001@server-id");
AccountGroupByIdAud expGroupAudit2 =
createExpGroupAudit(group.getId(), subgroupUuid2, userId, getTipTimestamp(uuid));
AccountGroupByIdAud expGroupAudit3 =
@@ -269,14 +204,6 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
// Add two account, including a removed account.
addMembers(uuid, ImmutableSet.of(id2, id3));
assertTipCommit(
uuid,
"Update group\n"
+ "\n"
+ String.format("Add: Account %s <%s@server-id>\n", id2, id2)
+ String.format("Add: Account %s <%s@server-id>", id3, id3),
"Account 100001",
"100001@server-id");
AccountGroupMemberAudit expMemberAudit4 =
createExpMemberAudit(groupId, id2, userId, getTipTimestamp(uuid));
AccountGroupMemberAudit expMemberAudit3 =
@@ -288,14 +215,6 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
// Remove two subgroups.
removeSubgroups(uuid, ImmutableSet.of(subgroupUuid1, subgroupUuid3));
assertTipCommit(
uuid,
"Update group\n"
+ "\n"
+ String.format("Remove-group: Group <%s>\n", subgroupUuid1)
+ String.format("Remove-group: Group <%s>", subgroupUuid3),
"Account 100001",
"100001@server-id");
expGroupAudit1.removed(userId, getTipTimestamp(uuid));
expGroupAudit3.removed(userId, getTipTimestamp(uuid));
assertThat(auditLogReader.getSubgroupsAudit(allUsersRepo, uuid))
@@ -334,81 +253,60 @@ public final class AuditLogReaderTest extends AbstractGroupTest {
GroupConfig groupConfig = GroupConfig.createForNewGroup(allUsersRepo, groupCreation);
groupConfig.setGroupUpdate(groupUpdate, getAuditLogFormatter());
RevCommit commit = groupConfig.commit(createMetaDataUpdate(authorIdent));
assertCreateGroup(authorIdent, commit);
groupConfig.commit(createMetaDataUpdate(authorIdent));
return groupConfig
.getLoadedGroup()
.orElseThrow(() -> new IllegalStateException("create group failed"));
}
private void assertCreateGroup(PersonIdent authorIdent, RevCommit commit) throws Exception {
if (authorIdent.equals(serverIdent)) {
assertServerCommit(CommitUtil.toCommitInfo(commit), "Create group");
} else {
String name = String.format("Account %s", userId);
String email = String.format("%s@%s", userId, SERVER_ID);
assertCommit(
CommitUtil.toCommitInfo(commit),
String.format("Create group\n\nAdd: %s <%s>", name, email),
name,
email);
}
}
private InternalGroup updateGroup(AccountGroup.UUID uuid, InternalGroupUpdate groupUpdate)
private void updateGroup(AccountGroup.UUID uuid, InternalGroupUpdate groupUpdate)
throws Exception {
GroupConfig groupConfig = GroupConfig.loadForGroup(allUsersRepo, uuid);
groupConfig.setGroupUpdate(groupUpdate, getAuditLogFormatter());
groupConfig.commit(createMetaDataUpdate(userIdent));
return groupConfig
.getLoadedGroup()
.orElseThrow(() -> new IllegalStateException("updated group failed"));
}
private InternalGroup addMembers(AccountGroup.UUID groupUuid, Set<Account.Id> ids)
throws Exception {
private void addMembers(AccountGroup.UUID groupUuid, Set<Account.Id> ids) throws Exception {
InternalGroupUpdate update =
InternalGroupUpdate.builder()
.setMemberModification(memberIds -> Sets.union(memberIds, ids))
.build();
return updateGroup(groupUuid, update);
updateGroup(groupUuid, update);
}
private InternalGroup removeMembers(AccountGroup.UUID groupUuid, Set<Account.Id> ids)
throws Exception {
private void removeMembers(AccountGroup.UUID groupUuid, Set<Account.Id> ids) throws Exception {
InternalGroupUpdate update =
InternalGroupUpdate.builder()
.setMemberModification(memberIds -> Sets.difference(memberIds, ids))
.build();
return updateGroup(groupUuid, update);
updateGroup(groupUuid, update);
}
private InternalGroup addSubgroups(AccountGroup.UUID groupUuid, Set<AccountGroup.UUID> uuids)
private void addSubgroups(AccountGroup.UUID groupUuid, Set<AccountGroup.UUID> uuids)
throws Exception {
InternalGroupUpdate update =
InternalGroupUpdate.builder()
.setSubgroupModification(memberIds -> Sets.union(memberIds, uuids))
.build();
return updateGroup(groupUuid, update);
updateGroup(groupUuid, update);
}
private InternalGroup removeSubgroups(AccountGroup.UUID groupUuid, Set<AccountGroup.UUID> uuids)
private void removeSubgroups(AccountGroup.UUID groupUuid, Set<AccountGroup.UUID> uuids)
throws Exception {
InternalGroupUpdate update =
InternalGroupUpdate.builder()
.setSubgroupModification(memberIds -> Sets.difference(memberIds, uuids))
.build();
return updateGroup(groupUuid, update);
updateGroup(groupUuid, update);
}
private AccountGroupMemberAudit createExpMemberAudit(
private static AccountGroupMemberAudit createExpMemberAudit(
AccountGroup.Id groupId, Account.Id id, Account.Id addedBy, Timestamp addedOn) {
return new AccountGroupMemberAudit(
new AccountGroupMemberAudit.Key(id, groupId, addedOn), addedBy);
}
private AccountGroupByIdAud createExpGroupAudit(
private static AccountGroupByIdAud createExpGroupAudit(
AccountGroup.Id groupId, AccountGroup.UUID uuid, Account.Id addedBy, Timestamp addedOn) {
return new AccountGroupByIdAud(new AccountGroupByIdAud.Key(groupId, uuid, addedOn), addedBy);
}