Don't modify All-Projects state in acceptance tests

We would like to reuse a running server across tests, so we shouldn't
modify All-Projects in ways that might affect later methods. Read-only
operations (e.g. listing child projects) are still generally ok, as
are idempotent modifications in setUp that are expected by all test
methods.

Change-Id: I3d47da484ae92339f326bb9344986d51730ca2de

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/edit/ChangeEditIT.java

@@ -34,6 +34,7 @@ import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.RestResponse;
 import com.google.gerrit.acceptance.RestSession;
 import com.google.gerrit.acceptance.TestProjectInput;
+import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.client.ListChangesOption;
 import com.google.gerrit.extensions.common.ApprovalInfo;
@@ -54,6 +55,7 @@ import com.google.gerrit.server.edit.ChangeEditUtil;
 import com.google.gerrit.server.edit.UnchangedCommitMessageException;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.project.InvalidChangeOperationException;
+import com.google.gerrit.server.project.Util;
 import com.google.gson.stream.JsonReader;
 import com.google.gwtorm.server.SchemaFactory;
 import com.google.inject.Inject;
@@ -623,10 +625,11 @@ public class ChangeEditIT extends AbstractDaemonTest {
   @Test
   public void editCommitMessageCopiesLabelScores() throws Exception {
     String cr = "Code-Review";
-    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
-    cfg.getLabelSections().get(cr)
+    LabelType codeReview = Util.codeReview();
-        .setCopyAllScoresIfNoCodeChange(true);
+    codeReview.setCopyAllScoresIfNoCodeChange(true);
-    saveProjectConfig(allProjects, cfg);
+    cfg.getLabelSections().put(cr, codeReview);
+    saveProjectConfig(project, cfg);
 
     String changeId = change.getKey().get();
     ReviewInput r = new ReviewInput();

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/DraftChangeBlockedIT.java

@@ -21,23 +21,19 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.common.data.Permission;
-import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.ProjectConfig;
 
 import org.junit.Before;
 import org.junit.Test;
 
-import java.io.IOException;
-
 @NoHttpd
 public class DraftChangeBlockedIT extends AbstractDaemonTest {
 
   @Before
   public void setUp() throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
     block(cfg, Permission.PUSH, ANONYMOUS_USERS, "refs/drafts/*");
-    saveProjectConfig(cfg);
+    saveProjectConfig(project, cfg);
-    projectCache.evict(cfg.getProject());
   }
 
   @Test
@@ -53,13 +49,4 @@ public class DraftChangeBlockedIT extends AbstractDaemonTest {
     PushOneCommit.Result r = pushTo("refs/for/master%draft");
     r.assertErrorStatus("cannot upload drafts");
   }
-
-  private void saveProjectConfig(ProjectConfig cfg) throws IOException {
-    MetaDataUpdate md = metaDataUpdateFactory.create(allProjects);
-    try {
-      cfg.commit(md);
-    } finally {
-      md.close();
-    }
-  }
 }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java

@@ -85,9 +85,9 @@ public class CreateBranchIT extends AbstractDaemonTest {
   }
 
   private void blockCreateReference() throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
     block(cfg, Permission.CREATE, ANONYMOUS_USERS, "refs/*");
-    saveProjectConfig(allProjects, cfg);
+    saveProjectConfig(project, cfg);
   }
 
   private void grantOwner() throws Exception {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java

@@ -77,9 +77,9 @@ public class DeleteBranchIT extends AbstractDaemonTest {
   }
 
   private void blockForcePush() throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
     block(cfg, Permission.PUSH, ANONYMOUS_USERS, "refs/heads/*").setForce(true);
-    saveProjectConfig(allProjects, cfg);
+    saveProjectConfig(project, cfg);
   }
 
   private void grantOwner() throws Exception {

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetCommitIT.java

@@ -15,14 +15,12 @@
 package com.google.gerrit.acceptance.rest.project;
 
 import static com.google.common.truth.Truth.assertThat;
-import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
 
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GitUtil;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.RestResponse;
-import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.common.CommitInfo;
 import com.google.gerrit.server.git.ProjectConfig;
@@ -42,12 +40,7 @@ public class GetCommitIT extends AbstractDaemonTest {
   @Before
   public void setUp() throws Exception {
     repo = GitUtil.newTestRepository(repoManager.openRepository(project));
-
+    blockRead(project, "refs/*");
-    ProjectConfig pc = projectCache.checkedGet(allProjects).getConfig();
-    for (AccessSection sec : pc.getAccessSections()) {
-      sec.removePermission(Permission.READ);
-    }
-    saveProjectConfig(allProjects, pc);
   }
 
   @After
@@ -65,7 +58,7 @@ public class GetCommitIT extends AbstractDaemonTest {
 
   @Test
   public void getMergedCommit_Found() throws Exception {
-    allow(Permission.READ, REGISTERED_USERS, "refs/heads/*");
+    unblockRead();
     RevCommit commit = repo.parseBody(repo.branch("master")
         .commit()
         .message("Create\n\nNew commit\n")
@@ -99,7 +92,7 @@ public class GetCommitIT extends AbstractDaemonTest {
 
   @Test
   public void getOpenChange_Found() throws Exception {
-    allow(Permission.READ, REGISTERED_USERS, "refs/heads/*");
+    unblockRead();
     PushOneCommit.Result r = pushFactory.create(db, admin.getIdent(), testRepo)
         .to("refs/for/master");
     r.assertOkStatus();
@@ -130,6 +123,12 @@ public class GetCommitIT extends AbstractDaemonTest {
     assertNotFound(r.getCommitId());
   }
 
+  private void unblockRead() throws Exception {
+    ProjectConfig pc = projectCache.checkedGet(project).getConfig();
+    pc.getAccessSection("refs/*").remove(new Permission(Permission.READ));
+    saveProjectConfig(project, pc);
+  }
+
   private void assertNotFound(ObjectId id) throws Exception {
     RestResponse r = userSession.get(
         "/projects/" + project.get() + "/commits/" + id.name());

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/ProjectLevelConfigIT.java

@@ -20,6 +20,7 @@ import static com.google.gerrit.acceptance.GitUtil.fetch;
 
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.server.project.ProjectState;
 
@@ -67,22 +68,28 @@ public class ProjectLevelConfigIT extends AbstractDaemonTest {
     parentCfg.setString("s2", "ss", "k3", "parentValue3");
     parentCfg.setString("s2", "ss", "k4", "parentValue4");
 
-    TestRepository<?> parentTestRepo = cloneProject(allProjects, sshSession);
+    pushFactory.create(
-    fetch(parentTestRepo, RefNames.REFS_CONFIG + ":refs/heads/config");
+          db, admin.getIdent(), testRepo, "Create Project Level Config",
-    parentTestRepo.reset("refs/heads/config");
+          configName, parentCfg.toText())
-    PushOneCommit push =
+        .to(RefNames.REFS_CONFIG)
-        pushFactory.create(db, admin.getIdent(), parentTestRepo, "Create Project Level Config",
+        .assertOkStatus();
-            configName, parentCfg.toText());
+
-    push.to(RefNames.REFS_CONFIG);
+    Project.NameKey childProject = createProject("child", project);
+    TestRepository<?> childTestRepo = cloneProject(childProject, sshSession);
+    fetch(childTestRepo, RefNames.REFS_CONFIG + ":refs/heads/config");
+    childTestRepo.reset("refs/heads/config");
 
     Config cfg = new Config();
     cfg.setString("s1", null, "k1", "childValue1");
     cfg.setString("s2", "ss", "k3", "childValue2");
-    push = pushFactory.create(db, admin.getIdent(), testRepo, "Create Project Level Config",
-        configName, cfg.toText());
-    push.to(RefNames.REFS_CONFIG);
 
-    ProjectState state = projectCache.get(project);
+    pushFactory.create(
+          db, admin.getIdent(), childTestRepo, "Create Project Level Config",
+          configName, cfg.toText())
+        .to(RefNames.REFS_CONFIG)
+        .assertOkStatus();
+
+    ProjectState state = projectCache.get(childProject);
 
     Config expectedCfg = new Config();
     expectedCfg.setString("s1", null, "k1", "childValue1");

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/CustomLabelIT.java

@@ -28,7 +28,6 @@ import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.reviewdb.client.AccountGroup;
-import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.group.SystemGroupBackend;
 import com.google.gerrit.server.project.Util;
@@ -39,29 +38,29 @@ import org.junit.Test;
 @NoHttpd
 public class CustomLabelIT extends AbstractDaemonTest {
 
-  private final LabelType Q = category("CustomLabel",
+  private final LabelType label = category("CustomLabel",
       value(1, "Positive"),
       value(0, "No score"),
       value(-1, "Negative"));
 
   @Before
   public void setUp() throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
     AccountGroup.UUID anonymousUsers =
         SystemGroupBackend.getGroup(ANONYMOUS_USERS).getUUID();
-    Util.allow(cfg, Permission.forLabel(Q.getName()), -1, 1, anonymousUsers,
+    Util.allow(cfg, Permission.forLabel(label.getName()), -1, 1, anonymousUsers,
         "refs/heads/*");
-    saveProjectConfig(cfg);
+    saveProjectConfig(project, cfg);
   }
 
   @Test
   public void customLabelNoOp_NegativeVoteNotBlock() throws Exception {
-    Q.setFunctionName("NoOp");
+    label.setFunctionName("NoOp");
     saveLabelConfig();
     PushOneCommit.Result r = createChange();
-    revision(r).review(new ReviewInput().label(Q.getName(), -1));
+    revision(r).review(new ReviewInput().label(label.getName(), -1));
     ChangeInfo c = get(r.getChangeId());
-    LabelInfo q = c.labels.get(Q.getName());
+    LabelInfo q = c.labels.get(label.getName());
     assertThat(q.all).hasSize(1);
     assertThat(q.rejected).isNotNull();
     assertThat(q.blocking).isNull();
@@ -69,12 +68,12 @@ public class CustomLabelIT extends AbstractDaemonTest {
 
   @Test
   public void customLabelNoBlock_NegativeVoteNotBlock() throws Exception {
-    Q.setFunctionName("NoBlock");
+    label.setFunctionName("NoBlock");
     saveLabelConfig();
     PushOneCommit.Result r = createChange();
-    revision(r).review(new ReviewInput().label(Q.getName(), -1));
+    revision(r).review(new ReviewInput().label(label.getName(), -1));
     ChangeInfo c = get(r.getChangeId());
-    LabelInfo q = c.labels.get(Q.getName());
+    LabelInfo q = c.labels.get(label.getName());
     assertThat(q.all).hasSize(1);
     assertThat(q.rejected).isNotNull();
     assertThat(q.blocking).isNull();
@@ -82,12 +81,12 @@ public class CustomLabelIT extends AbstractDaemonTest {
 
   @Test
   public void customLabelMaxNoBlock_NegativeVoteNotBlock() throws Exception {
-    Q.setFunctionName("MaxNoBlock");
+    label.setFunctionName("MaxNoBlock");
     saveLabelConfig();
     PushOneCommit.Result r = createChange();
-    revision(r).review(new ReviewInput().label(Q.getName(), -1));
+    revision(r).review(new ReviewInput().label(label.getName(), -1));
     ChangeInfo c = get(r.getChangeId());
-    LabelInfo q = c.labels.get(Q.getName());
+    LabelInfo q = c.labels.get(label.getName());
     assertThat(q.all).hasSize(1);
     assertThat(q.rejected).isNotNull();
     assertThat(q.blocking).isNull();
@@ -95,12 +94,12 @@ public class CustomLabelIT extends AbstractDaemonTest {
 
   @Test
   public void customLabelAnyWithBlock_NegativeVoteBlock() throws Exception {
-    Q.setFunctionName("AnyWithBlock");
+    label.setFunctionName("AnyWithBlock");
     saveLabelConfig();
     PushOneCommit.Result r = createChange();
-    revision(r).review(new ReviewInput().label(Q.getName(), -1));
+    revision(r).review(new ReviewInput().label(label.getName(), -1));
     ChangeInfo c = get(r.getChangeId());
-    LabelInfo q = c.labels.get(Q.getName());
+    LabelInfo q = c.labels.get(label.getName());
     assertThat(q.all).hasSize(1);
     assertThat(q.disliked).isNull();
     assertThat(q.rejected).isNotNull();
@@ -111,9 +110,9 @@ public class CustomLabelIT extends AbstractDaemonTest {
   public void customLabelMaxWithBlock_NegativeVoteBlock() throws Exception {
     saveLabelConfig();
     PushOneCommit.Result r = createChange();
-    revision(r).review(new ReviewInput().label(Q.getName(), -1));
+    revision(r).review(new ReviewInput().label(label.getName(), -1));
     ChangeInfo c = get(r.getChangeId());
-    LabelInfo q = c.labels.get(Q.getName());
+    LabelInfo q = c.labels.get(label.getName());
     assertThat(q.all).hasSize(1);
     assertThat(q.disliked).isNull();
     assertThat(q.rejected).isNotNull();
@@ -121,18 +120,8 @@ public class CustomLabelIT extends AbstractDaemonTest {
   }
 
   private void saveLabelConfig() throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
-    cfg.getLabelSections().put(Q.getName(), Q);
+    cfg.getLabelSections().put(label.getName(), label);
-    saveProjectConfig(cfg);
+    saveProjectConfig(project, cfg);
-  }
-
-  private void saveProjectConfig(ProjectConfig cfg) throws Exception {
-    MetaDataUpdate md = metaDataUpdateFactory.create(allProjects);
-    try {
-      cfg.commit(md);
-    } finally {
-      md.close();
-    }
-    projectCache.evict(allProjects);
   }
 }

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/project/LabelTypeIT.java

@@ -14,7 +14,6 @@
 
 package com.google.gerrit.acceptance.server.project;
 
-import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.gerrit.acceptance.AbstractDaemonTest;
@@ -28,6 +27,7 @@ import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.notedb.NotesMigration;
+import com.google.gerrit.server.project.Util;
 import com.google.gerrit.testutil.ConfigSuite;
 
 import org.eclipse.jgit.lib.Config;
@@ -46,15 +46,15 @@ public class LabelTypeIT extends AbstractDaemonTest {
 
   @Before
   public void setUp() throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
-    codeReview = checkNotNull(cfg.getLabelSections().get("Code-Review"));
+    codeReview = Util.codeReview();
     codeReview.setDefaultValue((short)-1);
+    cfg.getLabelSections().put(codeReview.getName(), codeReview);
     saveProjectConfig(cfg);
   }
 
   @Test
   public void noCopyMinScoreOnRework() throws Exception {
-    //allProjects only has it true by default
     codeReview.setCopyMinScore(false);
     saveLabelConfig();
 
@@ -312,14 +312,14 @@ public class LabelTypeIT extends AbstractDaemonTest {
   }
 
   private void saveLabelConfig() throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
     cfg.getLabelSections().clear();
     cfg.getLabelSections().put(codeReview.getName(), codeReview);
     saveProjectConfig(cfg);
   }
 
   private void saveProjectConfig(ProjectConfig cfg) throws Exception {
-    MetaDataUpdate md = metaDataUpdateFactory.create(allProjects);
+    MetaDataUpdate md = metaDataUpdateFactory.create(project);
     try {
       cfg.commit(md);
     } finally {

gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java

@@ -86,12 +86,14 @@ public class Util {
   public static final AccountGroup.UUID ADMIN = new AccountGroup.UUID("test.admin");
   public static final AccountGroup.UUID DEVS = new AccountGroup.UUID("test.devs");
 
-  public static final LabelType CR = category("Code-Review",
+  public static final LabelType codeReview() {
-      value(2, "Looks good to me, approved"),
+    return category("Code-Review",
-      value(1, "Looks good to me, but someone else must approve"),
+        value(2, "Looks good to me, approved"),
-      value(0, "No score"),
+        value(1, "Looks good to me, but someone else must approve"),
-      value(-1, "I would prefer this is not merged as is"),
+        value(0, "No score"),
-      value(-2, "This shall not be merged"));
+        value(-1, "I would prefer this is not merged as is"),
+        value(-2, "This shall not be merged"));
+  }
 
   public static LabelValue value(int value, String text) {
     return new LabelValue((short) value, text);
@@ -215,7 +217,8 @@ public class Util {
       Repository repo = repoManager.createRepository(allProjectsName);
       allProjects = new ProjectConfig(new Project.NameKey(allProjectsName.get()));
       allProjects.load(repo);
-      allProjects.getLabelSections().put(CR.getName(), CR);
+      LabelType cr = codeReview();
+      allProjects.getLabelSections().put(cr.getName(), cr);
       add(allProjects);
     } catch (IOException | ConfigInvalidException e) {
       throw new RuntimeException(e);