opendev/gerrit
Code Issues Proposed changes

Configure ProjectResetter through a plain data class.

Browse Source 
Introduce ProjectResetter.Config and use that for defining what to
reset. The old scheme used a Builder which used GUICE, which means the
Builder cannot be instantiated unless the GUICE injector is already
setup.

Change-Id: Ic20995d2e05a4b76d9048a3c8052f1c3b1d0d1a5
This commit is contained in:
Han-Wen Nienhuys
2018-02-21 17:50:10 +01:00
parent af3cd51f10
commit 958b481147
6 changed files with 78 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
java/com/google/gerrit/acceptance/AbstractDaemonTest.java
View File

@@ -201,7 +201,12 @@ public abstract class AbstractDaemonTest {
firstTest = description;
}
beforeTest(description);
try (ProjectResetter resetter = resetProjects(projectResetter.builder())) {
ProjectResetter.Config input = resetProjects();
if (input == null) {
input = defaultResetProjects();
}
try (ProjectResetter resetter = projectResetter.builder().build(input)) {
AbstractDaemonTest.this.resetter = resetter;
base.evaluate();
} finally {
@@ -317,8 +322,12 @@ public abstract class AbstractDaemonTest {
}
/** Controls which project and branches should be reset after each test case. */
protected ProjectResetter resetProjects(ProjectResetter.Builder resetter) throws IOException {
return resetter
protected ProjectResetter.Config resetProjects() {
return null;
}
private ProjectResetter.Config defaultResetProjects() {
return new ProjectResetter.Config()
// Don't reset all refs so that refs/sequences/changes is not touched and change IDs are
// not reused.
.reset(allProjects, RefNames.REFS_CONFIG)
@@ -331,8 +340,7 @@ public abstract class AbstractDaemonTest {
RefNames.REFS_USERS + "*",
RefNames.REFS_EXTERNAL_IDS,
RefNames.REFS_STARRED_CHANGES + "*",
RefNames.REFS_DRAFT_COMMENTS + "*")
.build();
RefNames.REFS_DRAFT_COMMENTS + "*");
}
protected void restartAsSlave() throws Exception {

5
java/com/google/gerrit/acceptance/AbstractNotificationTest.java
View File

@@ -25,7 +25,6 @@ import com.google.common.collect.ImmutableList;
import com.google.common.truth.FailureMetadata;
import com.google.common.truth.Subject;
import com.google.common.truth.Truth;
import com.google.gerrit.acceptance.ProjectResetter.Builder;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -63,10 +62,10 @@ public abstract class AbstractNotificationTest extends AbstractDaemonTest {
}
@Override
protected ProjectResetter resetProjects(Builder resetter) throws IOException {
protected ProjectResetter.Config resetProjects() {
// Don't reset anything so that stagedUsers can be cached across all tests.
// Without this caching these tests become much too slow.
return resetter.build();
return new ProjectResetter.Config();
}
protected static FakeEmailSenderSubject assertThat(FakeEmailSender sender) {

26
java/com/google/gerrit/acceptance/ProjectResetter.java
View File

@@ -88,8 +88,6 @@ public class ProjectResetter implements AutoCloseable {
@Nullable private final AccountCache accountCache;
@Nullable private final ProjectCache projectCache;
private final Multimap<Project.NameKey, String> refsByProject;
@Inject
public Builder(
GitRepositoryManager repoManager,
@@ -102,10 +100,27 @@ public class ProjectResetter implements AutoCloseable {
this.accountCreator = accountCreator;
this.accountCache = accountCache;
this.projectCache = projectCache;
}
public ProjectResetter build(ProjectResetter.Config input) throws IOException {
return new ProjectResetter(
repoManager,
allUsersName,
accountCreator,
accountCache,
projectCache,
input.refsByProject);
}
}
public static class Config {
private final Multimap<Project.NameKey, String> refsByProject;
public Config() {
this.refsByProject = MultimapBuilder.hashKeys().arrayListValues().build();
}
public Builder reset(Project.NameKey project, String... refPatterns) {
public Config reset(Project.NameKey project, String... refPatterns) {
List<String> refPatternList = Arrays.asList(refPatterns);
if (refPatternList.isEmpty()) {
refPatternList = ImmutableList.of(RefNames.REFS + "*");
@@ -113,11 +128,6 @@ public class ProjectResetter implements AutoCloseable {
refsByProject.putAll(project, refPatternList);
return this;
}
public ProjectResetter build() throws IOException {
return new ProjectResetter(
repoManager, allUsersName, accountCreator, accountCache, projectCache, refsByProject);
}
}
@Inject private GitRepositoryManager repoManager;

50
javatests/com/google/gerrit/acceptance/ProjectResetterTest.java
View File

@@ -71,7 +71,8 @@ public class ProjectResetterTest extends GerritBaseTests {
public void resetAllRefs() throws Exception {
Ref matchingRef = createRef("refs/any/test");
try (ProjectResetter resetProject = builder().reset(project).build()) {
try (ProjectResetter resetProject =
builder().build(new ProjectResetter.Config().reset(project))) {
updateRef(matchingRef);
}
@@ -87,7 +88,10 @@ public class ProjectResetterTest extends GerritBaseTests {
Ref updatedNonMatchingRef;
try (ProjectResetter resetProject =
builder().reset(project, "refs/match/*", "refs/another-match/*").build()) {
builder()
.build(
new ProjectResetter.Config()
.reset(project, "refs/match/*", "refs/another-match/*"))) {
updateRef(matchingRef);
updateRef(anotherMatchingRef);
updatedNonMatchingRef = updateRef(nonMatchingRef);
@@ -107,7 +111,10 @@ public class ProjectResetterTest extends GerritBaseTests {
Ref anotherMatchingRef;
Ref nonMatchingRef;
try (ProjectResetter resetProject =
builder().reset(project, "refs/match/*", "refs/another-match/*").build()) {
builder()
.build(
new ProjectResetter.Config()
.reset(project, "refs/match/*", "refs/another-match/*"))) {
matchingRef = createRef("refs/match/test");
anotherMatchingRef = createRef("refs/another-match/test");
nonMatchingRef = createRef("refs/no-match/test");
@@ -135,7 +142,11 @@ public class ProjectResetterTest extends GerritBaseTests {
Ref updatedNonMatchingRefProject1;
Ref updatedNonMatchingRefProject2;
try (ProjectResetter resetProject =
builder().reset(project, "refs/foo/*").reset(project2, "refs/bar/*").build()) {
builder()
.build(
new ProjectResetter.Config()
.reset(project, "refs/foo/*")
.reset(project2, "refs/bar/*"))) {
updateRef(matchingRefProject1);
updatedNonMatchingRefProject1 = updateRef(nonMatchingRefProject1);
@@ -162,7 +173,11 @@ public class ProjectResetterTest extends GerritBaseTests {
Ref matchingRefProject2;
Ref nonMatchingRefProject2;
try (ProjectResetter resetProject =
builder().reset(project, "refs/foo/*").reset(project2, "refs/bar/*").build()) {
builder()
.build(
new ProjectResetter.Config()
.reset(project, "refs/foo/*")
.reset(project2, "refs/bar/*"))) {
matchingRefProject1 = createRef("refs/foo/test");
nonMatchingRefProject1 = createRef("refs/bar/test");
@@ -183,7 +198,9 @@ public class ProjectResetterTest extends GerritBaseTests {
public void onlyDeleteNewlyCreatedWithOverlappingRefPatterns() throws Exception {
Ref matchingRef;
try (ProjectResetter resetProject =
builder().reset(project, "refs/match/*", "refs/match/test").build()) {
builder()
.build(
new ProjectResetter.Config().reset(project, "refs/match/*", "refs/match/test"))) {
// This ref matches 2 ref pattern, ProjectResetter should try to delete it only once.
matchingRef = createRef("refs/match/test");
}
@@ -206,7 +223,8 @@ public class ProjectResetterTest extends GerritBaseTests {
Ref nonMetaConfig = createRef("refs/heads/master");
try (ProjectResetter resetProject =
builder(null, null, projectCache).reset(project).reset(project2).build()) {
builder(null, null, projectCache)
.build(new ProjectResetter.Config().reset(project).reset(project2))) {
updateRef(nonMetaConfig);
updateRef(repo2, metaConfig);
}
@@ -225,7 +243,8 @@ public class ProjectResetterTest extends GerritBaseTests {
EasyMock.replay(projectCache);
try (ProjectResetter resetProject =
builder(null, null, projectCache).reset(project).reset(project2).build()) {
builder(null, null, projectCache)
.build(new ProjectResetter.Config().reset(project).reset(project2))) {
createRef("refs/heads/master");
createRef(repo2, RefNames.REFS_CONFIG);
}
@@ -249,7 +268,8 @@ public class ProjectResetterTest extends GerritBaseTests {
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(2)));
try (ProjectResetter resetProject =
builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
builder(null, accountCache, null)
.build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
updateRef(allUsersRepo, userBranch);
}
@@ -269,7 +289,8 @@ public class ProjectResetterTest extends GerritBaseTests {
EasyMock.replay(accountCache);
try (ProjectResetter resetProject =
builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
builder(null, accountCache, null)
.build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
// Non-user branch because it's not in All-Users.
createRef(RefNames.refsUsers(new Account.Id(2)));
@@ -300,7 +321,8 @@ public class ProjectResetterTest extends GerritBaseTests {
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(3)));
try (ProjectResetter resetProject =
builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
builder(null, accountCache, null)
.build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
updateRef(allUsersRepo, externalIds);
createRef(allUsersRepo, RefNames.refsUsers(accountId2));
@@ -329,7 +351,8 @@ public class ProjectResetterTest extends GerritBaseTests {
Ref nonUserBranch = createRef(RefNames.refsUsers(new Account.Id(3)));
try (ProjectResetter resetProject =
builder(null, accountCache, null).reset(project).reset(allUsers).build()) {
builder(null, accountCache, null)
.build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
updateRef(nonUserBranch);
createRef(allUsersRepo, RefNames.REFS_EXTERNAL_IDS);
createRef(allUsersRepo, RefNames.refsUsers(accountId2));
@@ -350,7 +373,8 @@ public class ProjectResetterTest extends GerritBaseTests {
EasyMock.replay(accountCreator);
try (ProjectResetter resetProject =
builder(accountCreator, null, null).reset(project).reset(allUsers).build()) {
builder(accountCreator, null, null)
.build(new ProjectResetter.Config().reset(project).reset(allUsers))) {
createRef(allUsersRepo, RefNames.refsUsers(accountId));
}

9
javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
View File

@@ -40,7 +40,6 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GerritConfig;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.ProjectResetter;
import com.google.gerrit.acceptance.ProjectResetter.Builder;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.Sandboxed;
import com.google.gerrit.acceptance.TestAccount;
@@ -166,11 +165,11 @@ public class GroupsIT extends AbstractDaemonTest {
}
@Override
protected ProjectResetter resetProjects(Builder resetter) throws IOException {
protected ProjectResetter.Config resetProjects() {
// Don't reset All-Users since deleting users makes groups inconsistent (e.g. groups would
// contain members that no longer exist) and as result of this the group consistency checker
// that is executed after each test would fail.
return resetter.reset(allProjects, RefNames.REFS_CONFIG).build();
return new ProjectResetter.Config().reset(allProjects, RefNames.REFS_CONFIG);
}
@Test
@@ -1076,7 +1075,9 @@ public class GroupsIT extends AbstractDaemonTest {
// Use ProjectResetter to restore the group names ref
try (ProjectResetter resetter =
projectResetter.builder().reset(allUsers, RefNames.REFS_GROUPNAMES).build()) {
projectResetter
.builder()
.build(new ProjectResetter.Config().reset(allUsers, RefNames.REFS_GROUPNAMES))) {
// Manually delete group names ref
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {

5
javatests/com/google/gerrit/acceptance/git/RefAdvertisementIT.java
View File

@@ -785,7 +785,8 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
private ProjectResetter resetGroups() throws IOException {
return projectResetter
.builder()
.reset(allUsers, RefNames.REFS_GROUPS + "*", RefNames.REFS_GROUPNAMES)
.build();
.build(
new ProjectResetter.Config()
.reset(allUsers, RefNames.REFS_GROUPS + "*", RefNames.REFS_GROUPNAMES));
}
}