Add more tests for submodule subscriptions

1. Different submit strategies are added for submodule testing
2. Same submodule exists in different paths
3. Standalone project with other submodule related projects in same
topic

Change-Id: I472bc2e831a41a9c4f53fe1ee0712d39b3bc404f

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -42,6 +42,7 @@ import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.api.projects.ProjectInput;
 import com.google.gerrit.extensions.client.InheritableBoolean;
 import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.extensions.common.ActionInfo;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.common.EditInfo;
@@ -415,15 +416,28 @@ public abstract class AbstractDaemonTest {
   protected Project.NameKey createProject(String nameSuffix,
       Project.NameKey parent) throws RestApiException {
     // Default for createEmptyCommit should match TestProjectConfig.
-    return createProject(nameSuffix, parent, true);
+    return createProject(nameSuffix, parent, true, null);
   }
 
   protected Project.NameKey createProject(String nameSuffix,
-      Project.NameKey parent, boolean createEmptyCommit)
+      Project.NameKey parent, boolean createEmptyCommit) throws RestApiException {
+    // Default for createEmptyCommit should match TestProjectConfig.
+    return createProject(nameSuffix, parent, createEmptyCommit, null);
+  }
+
+  protected Project.NameKey createProject(String nameSuffix,
+      Project.NameKey parent, SubmitType submitType) throws RestApiException {
+    // Default for createEmptyCommit should match TestProjectConfig.
+    return createProject(nameSuffix, parent, true, submitType);
+  }
+
+  protected Project.NameKey createProject(String nameSuffix,
+      Project.NameKey parent, boolean createEmptyCommit, SubmitType submitType)
       throws RestApiException {
     ProjectInput in = new ProjectInput();
     in.name = name(nameSuffix);
     in.parent = parent != null ? parent.get() : null;
+    in.submitType = submitType;
     in.createEmptyCommit = createEmptyCommit;
     return createProject(in);
   }

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

@@ -21,6 +21,7 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.common.data.SubscribeSection;
+import com.google.gerrit.extensions.client.SubmitType;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.git.ProjectConfig;
@@ -40,14 +41,52 @@ import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
 import java.util.concurrent.atomic.AtomicInteger;
 
 public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
+
+  protected SubmitType getSubmitType() {
+    return cfg.getEnum("project", null, "submitType", SubmitType.MERGE_IF_NECESSARY);
+  }
+
+  protected static Config submitByMergeAlways() {
+    Config cfg = new Config();
+    cfg.setBoolean("change", null, "submitWholeTopic", true);
+    cfg.setEnum("project", null, "submitType", SubmitType.MERGE_ALWAYS);
+    return cfg;
+  }
+
+  protected static Config submitByMergeIfNecessary() {
+    Config cfg = new Config();
+    cfg.setBoolean("change", null, "submitWholeTopic", true);
+    cfg.setEnum("project", null, "submitType", SubmitType.MERGE_IF_NECESSARY);
+    return cfg;
+  }
+
+  protected static Config submitByCherryPickConifg() {
+    Config cfg = new Config();
+    cfg.setBoolean("change", null, "submitWholeTopic", true);
+    cfg.setEnum("project", null, "submitType", SubmitType.CHERRY_PICK);
+    return cfg;
+  }
+
+  protected static Config submitByRebaseConifg() {
+    Config cfg = new Config();
+    cfg.setBoolean("change", null, "submitWholeTopic", true);
+    cfg.setEnum("project", null, "submitType", SubmitType.REBASE_IF_NECESSARY);
+    return cfg;
+  }
+
   protected TestRepository<?> createProjectWithPush(String name,
-      @Nullable Project.NameKey parent) throws Exception {
-    Project.NameKey project = createProject(name, parent);
+      @Nullable Project.NameKey parent, SubmitType submitType) throws Exception {
+    Project.NameKey project = createProject(name, parent, submitType);
     grant(Permission.PUSH, project, "refs/heads/*");
     grant(Permission.SUBMIT, project, "refs/for/refs/heads/*");
     return cloneProject(project);
   }
 
+  protected TestRepository<?> createProjectWithPush(String name,
+      @Nullable Project.NameKey parent) throws Exception {
+    return createProjectWithPush(name, parent, getSubmitType());
+  }
+
   protected TestRepository<?> createProjectWithPush(String name)
       throws Exception {
     return createProjectWithPush(name, null);
@@ -56,10 +95,11 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
   private static AtomicInteger contentCounter = new AtomicInteger(0);
 
   protected ObjectId pushChangeTo(TestRepository<?> repo, String ref,
-      String message, String topic) throws Exception {
+      String file, String content, String message, String topic)
+      throws Exception {
     ObjectId ret = repo.branch("HEAD").commit().insertChangeId()
       .message(message)
-      .add("a.txt", "a contents: " + contentCounter.incrementAndGet())
+      .add(file, content)
       .create();
 
     String pushedRef = ref;
@@ -79,6 +119,12 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
     return ret;
   }
 
+  protected ObjectId pushChangeTo(TestRepository<?> repo, String ref,
+      String message, String topic) throws Exception {
+    return pushChangeTo(repo, ref, "a.txt",
+        "a contents: " + contentCounter.incrementAndGet(), message, topic);
+  }
+
   protected ObjectId pushChangeTo(TestRepository<?> repo, String branch)
       throws Exception {
     return pushChangeTo(repo, "refs/heads/" + branch, "some change", "");
@@ -135,16 +181,25 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
 
   protected void prepareSubmoduleConfigEntry(Config config,
       String subscribeToRepo, String subscribeToBranch) {
+    // The submodule subscription module checks for gerrit.canonicalWebUrl to
+    // detect if it's configured for automatic updates. It doesn't matter if
+    // it serves from that URL.
+    prepareSubmoduleConfigEntry(config, subscribeToRepo, subscribeToRepo, subscribeToBranch);
+  }
+
+  protected void prepareSubmoduleConfigEntry(Config config,
+      String subscribeToRepo, String subscribeToRepoPath, String subscribeToBranch) {
     subscribeToRepo = name(subscribeToRepo);
+    subscribeToRepoPath = name(subscribeToRepoPath);
     // The submodule subscription module checks for gerrit.canonicalWebUrl to
     // detect if it's configured for automatic updates. It doesn't matter if
     // it serves from that URL.
     String url = cfg.getString("gerrit", null, "canonicalWebUrl") + "/"
         + subscribeToRepo;
-    config.setString("submodule", subscribeToRepo, "path", subscribeToRepo);
-    config.setString("submodule", subscribeToRepo, "url", url);
+    config.setString("submodule", subscribeToRepoPath, "path", subscribeToRepoPath);
+    config.setString("submodule", subscribeToRepoPath, "url", url);
     if (subscribeToBranch != null) {
-      config.setString("submodule", subscribeToRepo, "branch", subscribeToBranch);
+      config.setString("submodule", subscribeToRepoPath, "branch", subscribeToBranch);
     }
   }
 
@@ -160,6 +215,27 @@ public abstract class AbstractSubmoduleSubscription extends AbstractDaemonTest {
         new RefSpec("HEAD:refs/heads/" + branch)).call();
   }
 
+  protected void expectToHaveSubmoduleState(TestRepository<?> repo,
+      String branch, String submodule, TestRepository<?> subRepo,
+      String subBranch) throws Exception {
+
+    submodule = name(submodule);
+    ObjectId commitId = repo.git().fetch().setRemote("origin").call()
+        .getAdvertisedRef("refs/heads/" + branch).getObjectId();
+
+    ObjectId subHead = subRepo.git().fetch().setRemote("origin").call()
+        .getAdvertisedRef("refs/heads/" + subBranch).getObjectId();
+
+    RevWalk rw = repo.getRevWalk();
+    RevCommit c = rw.parseCommit(commitId);
+    rw.parseBody(c.getTree());
+
+    RevTree tree = c.getTree();
+    RevObject actualId = repo.get(tree, submodule);
+
+    assertThat(actualId).isEqualTo(subHead);
+  }
+
   protected void expectToHaveSubmoduleState(TestRepository<?> repo,
       String branch, String submodule, ObjectId expectedId) throws Exception {
 

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

@@ -20,6 +20,7 @@ import static com.google.gerrit.acceptance.GitUtil.getChangeId;
 
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.ChangeStatus;
 import com.google.gerrit.testutil.ConfigSuite;
 
 import org.eclipse.jgit.junit.TestRepository;
@@ -34,8 +35,23 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
   extends AbstractSubmoduleSubscription {
 
   @ConfigSuite.Default
-  public static Config submitWholeTopicEnabled() {
-    return submitWholeTopicEnabledConfig();
+  public static Config mergeIfNecessary() {
+    return submitByMergeIfNecessary();
+  }
+
+  @ConfigSuite.Config
+  public static Config mergeAlways() {
+    return submitByMergeAlways();
+  }
+
+  @ConfigSuite.Config
+  public static Config cherryPick() {
+    return submitByCherryPickConifg();
+  }
+
+  @ConfigSuite.Config
+  public static Config rebase() {
+    return submitByRebaseConifg();
   }
 
   @Test
@@ -193,9 +209,82 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
 
     gApi.changes().id(getChangeId(sub1, sub1Id).get()).current().submit();
 
-    expectToHaveSubmoduleState(superRepo, "master", "sub1", sub1Id);
-    expectToHaveSubmoduleState(superRepo, "master", "sub2", sub2Id);
-    expectToHaveSubmoduleState(superRepo, "master", "sub3", sub3Id);
+    expectToHaveSubmoduleState(superRepo, "master", "sub1", sub1, "master");
+    expectToHaveSubmoduleState(superRepo, "master", "sub2", sub2, "master");
+    expectToHaveSubmoduleState(superRepo, "master", "sub3", sub3, "master");
+
+    superRepo.git().fetch().setRemote("origin").call()
+      .getAdvertisedRef("refs/heads/master").getObjectId();
+
+    assertWithMessage("submodule subscription update "
+        + "should have made one commit")
+        .that(superRepo.getRepository().resolve("origin/master^"))
+        .isEqualTo(superPreviousId);
+  }
+
+  @Test
+  public void testDifferentPaths() throws Exception {
+    TestRepository<?> superRepo = createProjectWithPush("super-project");
+    TestRepository<?> sub = createProjectWithPush("sub");
+
+    allowSubmoduleSubscription("sub", "refs/heads/master",
+        "super-project", "refs/heads/master");
+
+    Config config = new Config();
+    prepareSubmoduleConfigEntry(config, "sub", "master");
+    prepareSubmoduleConfigEntry(config, "sub", "sub-copy", "master");
+    pushSubmoduleConfig(superRepo, "master", config);
+
+    ObjectId superPreviousId = pushChangeTo(superRepo, "master");
+
+    ObjectId subId = pushChangeTo(sub, "refs/for/master", "some message", "");
+
+    approve(getChangeId(sub, subId).get());
+
+    gApi.changes().id(getChangeId(sub, subId).get()).current().submit();
+
+    expectToHaveSubmoduleState(superRepo, "master", "sub", sub, "master");
+    expectToHaveSubmoduleState(superRepo, "master", "sub-copy", sub, "master");
+
+    superRepo.git().fetch().setRemote("origin").call()
+        .getAdvertisedRef("refs/heads/master").getObjectId();
+
+    assertWithMessage("submodule subscription update "
+        + "should have made one commit")
+        .that(superRepo.getRepository().resolve("origin/master^"))
+        .isEqualTo(superPreviousId);
+  }
+
+  @Test
+  public void testNonSubmoduleInSameTopic() throws Exception {
+    TestRepository<?> superRepo = createProjectWithPush("super-project");
+    TestRepository<?> sub = createProjectWithPush("sub");
+    TestRepository<?> standAlone = createProjectWithPush("standalone");
+
+    allowSubmoduleSubscription("sub", "refs/heads/master",
+        "super-project", "refs/heads/master");
+
+    createSubmoduleSubscription(superRepo, "master", "sub", "master");
+
+    ObjectId superPreviousId = pushChangeTo(superRepo, "master");
+
+    ObjectId subId =
+        pushChangeTo(sub, "refs/for/master", "some message", "same-topic");
+    ObjectId standAloneId =
+        pushChangeTo(standAlone, "refs/for/master", "some message",
+            "same-topic");
+
+    String subChangeId = getChangeId(sub, subId).get();
+    String standAloneChangeId = getChangeId(standAlone, standAloneId).get();
+    approve(subChangeId);
+    approve(standAloneChangeId);
+
+    gApi.changes().id(subChangeId).current().submit();
+
+    expectToHaveSubmoduleState(superRepo, "master", "sub", sub, "master");
+
+    ChangeStatus status = gApi.changes().id(standAloneChangeId).info().status;
+    assertThat(status).isEqualTo(ChangeStatus.MERGED);
 
     superRepo.git().fetch().setRemote("origin").call()
         .getAdvertisedRef("refs/heads/master").getObjectId();
@@ -233,8 +322,8 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
 
     gApi.changes().id(id1).current().submit();
 
-    assertThat(hasSubmodule(midRepo, "master", "bottom-project")).isTrue();
-    assertThat(hasSubmodule(topRepo, "master", "mid-project")).isTrue();
+    expectToHaveSubmoduleState(midRepo, "master", "bottom-project", bottomRepo, "master");
+    expectToHaveSubmoduleState(topRepo, "master", "mid-project", midRepo, "master");
   }
 
   @Test
@@ -269,9 +358,9 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
 
     gApi.changes().id(id1).current().submit();
 
-    assertThat(hasSubmodule(midRepo, "master", "bottom-project")).isTrue();
-    assertThat(hasSubmodule(topRepo, "master", "mid-project")).isTrue();
-    assertThat(hasSubmodule(topRepo, "master", "bottom-project")).isTrue();
+    expectToHaveSubmoduleState(midRepo, "master", "bottom-project", bottomRepo, "master");
+    expectToHaveSubmoduleState(topRepo, "master", "mid-project", midRepo, "master");
+    expectToHaveSubmoduleState(topRepo, "master", "bottom-project", bottomRepo, "master");
   }
 
   @Test
@@ -317,8 +406,6 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
     allowSubmoduleSubscription("super-project", "refs/heads/dev",
         "subscribed-to-project", "refs/heads/dev");
 
-    pushChangeTo(subRepo, "master");
-    pushChangeTo(superRepo, "master");
     pushChangeTo(subRepo, "dev");
     pushChangeTo(superRepo, "dev");
 
@@ -327,9 +414,11 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
     createSubmoduleSubscription(subRepo, "dev", "super-project", "dev");
 
     ObjectId subMasterHead =
-        pushChangeTo(subRepo, "refs/for/master", "some message", "same-topic");
+        pushChangeTo(subRepo, "refs/for/master", "b.txt", "content b",
+            "some message", "same-topic");
     ObjectId superDevHead =
-        pushChangeTo(superRepo, "refs/for/dev", "some message", "same-topic");
+        pushChangeTo(superRepo, "refs/for/dev",
+            "some message", "same-topic");
 
     approve(getChangeId(subRepo, subMasterHead).get());
     approve(getChangeId(superRepo, superDevHead).get());
@@ -340,9 +429,8 @@ public class SubmoduleSubscriptionsWholeTopicMergeIT
     gApi.changes().id(getChangeId(subRepo, subMasterHead).get()).current()
         .submit();
 
-    assertThat(hasSubmodule(superRepo, "master",
-        "subscribed-to-project")).isFalse();
-    assertThat(hasSubmodule(subRepo, "dev",
-        "super-project")).isFalse();
+    assertThat(hasSubmodule(superRepo, "master", "subscribed-to-project"))
+        .isFalse();
+    assertThat(hasSubmodule(subRepo, "dev", "super-project")).isFalse();
   }
 }