First submodule test working

Author: shs96c

tests/integration/java/com/github/bazel_contrib/target_determinator/integration/Commits.java

@@ -61,14 +61,6 @@ class Commits {
     public static final String REDUCE_DEPENDENCY_VISIBILITY = "reduce_dependency_visibility";
     public static final String ONE_TEST_WITH_GITIGNORE = "one_test_with_gitignore";
     public static final String TWO_TESTS_WITH_GITIGNORE = "two_tests_with_gitignore";
-    public static final String SUBMODULE_ADD_TRIVIAL_SUBMODULE =
-            "b88ddcfe3da63c8308ce6d3274dd424d2c7b211a";
-    public static final String SUBMODULE_ADD_DEPENDENT_ON_SIMPLE_JAVA_LIBRARY =
-            "4e9b396b3a8030925d7b544cda3f1edbc199810f";
-    public static final String SUBMODULE_CHANGE_DIRECTORY =
-            "1cef87480c2c1dac74cc9de7470504fbd2b80265";
-    public static final String SUBMODULE_DELETE_SUBMODULE =
-            "d1b1d8f07f2e99429bafda134282b97588c69b3d";
     public static final String TWO_TESTS_BRANCH =
             "two-tests-branch"; // Local only (created by the test case).
     public static final String ONE_SH_TEST = "one_sh_test";

tests/integration/java/com/github/bazel_contrib/target_determinator/integration/TargetDeterminatorSpecificFlagsTest.java

@@ -207,7 +207,7 @@ public void failForUncleanSubmoduleWithEnforceClean() throws Exception {
 
     String beforeCommit = repo.commit("Add dependent target in submodule", "demo-submodule");
 
-    repo.move("demo-submodule", "demo-submodule-2");
+    repo.moveSubmodule("demo-submodule", "demo-submodule-2");
     repo.commit("Move demo-submodule to demo-submodule-2");
 
     Files.createFile(repo.getDir().resolve("demo-submodule-2").resolve("untracked-file"));

tests/integration/java/com/github/bazel_contrib/target_determinator/integration/TestRepo.java

@@ -21,6 +21,7 @@
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.Comparator;
+import java.util.List;
 import java.util.Objects;
 import java.util.Set;
 import java.util.function.Predicate;
@@ -200,11 +201,35 @@ public void pull() {
         }
     }
 
-    public void move(String from, String to) {
+    public void moveSubmodule(String from, String to) {
         try {
             Files.move(dir.resolve(from), dir.resolve(to));
-        } catch (IOException e) {
-            throw new UncheckedIOException(e);
+
+            // TODO: Using `String.replace` the way we are is asking for trouble. Do this nicely.
+
+            // Check the .gitmodules file if necessary
+            Path gitModules = dir.resolve(".gitmodules");
+            String contents = Files.readString(gitModules);
+            contents = contents.replace("path = " + from, "path = " + to);
+            Files.writeString(gitModules, contents);
+
+            gitRepo.add().addFilepattern(to).call();
+            gitRepo.rm().addFilepattern(from).setCached(true).call();
+
+            Path newGitSubmoduleDir = dir.resolve(".git").resolve("modules").resolve(to);
+            Files.move(dir.resolve(".git").resolve("modules").resolve(from), newGitSubmoduleDir);
+
+            Path submoduleConfig = newGitSubmoduleDir.resolve("config");
+            String configContents = Files.readString(submoduleConfig);
+            configContents = configContents.replace("/" + from, "/" + to);
+            Files.writeString(submoduleConfig, configContents);
+
+            Path gitFile = dir.resolve(to).resolve(".git");
+            String gitFileContents = Files.readString(gitFile);
+            gitFileContents = gitFileContents.replace("/" + from, "/" + to);
+            Files.writeString(gitFile, gitFileContents);
+        } catch (IOException | GitAPIException e) {
+            throw new RuntimeException(e);
         }
     }
 
@@ -225,4 +250,43 @@ public String getBranch() {
             throw new UncheckedIOException(e);
         }
     }
+
+    public void removeSubmodule(String submodulePath) {
+        try {
+            FileBasedConfig config = (FileBasedConfig) gitRepo.getRepository().getConfig();
+            config.unsetSection("submodule", submodulePath);
+            config.save();
+
+            // Remove the submodule from the `.gitmodules` file
+            deleteFromGitsubmodulesFile(submodulePath);
+
+            gitRepo.rm().addFilepattern(submodulePath).setCached(true).call();
+        } catch (GitAPIException | IOException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    private void deleteFromGitsubmodulesFile(String submodulePath) throws IOException {
+        Path gitModules = dir.resolve(".gitmodules");
+        List<String> contents = Files.readAllLines(gitModules);
+        StringBuilder newContents = new StringBuilder();
+        boolean inSection = false;
+        for (String line : contents) {
+            // New sections start at column 0, with something like
+            // `[submodule`. Look for the starter character.
+            if (inSection && !line.isEmpty() && line.charAt(0) == '[') {
+                inSection = false;
+            }
+            // However, the section we want to delete starts with a very
+            // particular pattern, so look for that next.
+            if (!inSection && line.startsWith("[submodule \"" + submodulePath + "\"]")) {
+                inSection = true;
+            }
+            // If we're not deleting things, copy it into the new contents
+            if (!inSection) {
+                newContents.append(line).append("\n");
+            }
+        }
+        Files.writeString(gitModules, newContents.toString());
+    }
 }

tests/integration/java/com/github/bazel_contrib/target_determinator/integration/Tests.java

@@ -2,6 +2,7 @@
 
 import static junit.framework.TestCase.fail;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assume.assumeFalse;
 
@@ -31,8 +32,15 @@
  */
 public abstract class Tests {
 
-  @Rule
-  public TemporaryFolder tempDir = new TemporaryFolder();
+  @Rule public TestName name = new TestName();
+  @Rule public TemporaryFolder tempDir = new TemporaryFolder();
+
+  protected Path testDir;
+
+  protected static final String ignoredDirectoryName = "ignored-directory";
+  protected static final String ignoredFileName = "some-file";
+
+  private boolean allowOverBuilds = false;
 
   /**
    * Get the targets affected by the diff between two commits.
@@ -46,15 +54,6 @@ public abstract class Tests {
   abstract Set<Label> getTargets(Path workspace, String commitBefore, String commitAfter)
       throws TargetComputationErrorException;
 
-  protected Path testDir;
-
-  protected static final String ignoredDirectoryName = "ignored-directory";
-  protected static final String ignoredFileName = "some-file";
-
-  private boolean allowOverBuilds = false;
-
-  @Rule public TestName name = new TestName();
-
   private Set<Label> getTargets(String commitBefore, String commitAfter)
       throws TargetComputationErrorException {
     return getTargets(testDir, commitBefore, commitAfter);
@@ -480,7 +479,7 @@ public void succeedForUncleanSubmodule() throws Exception {
     submoduleWithinRepo.pull();
     String beforeCommit = repo.commit("Add dependent targets in submodule", "demo-submodule");
 
-    repo.move("demo-submodule", "demo-submodule-2");
+    repo.moveSubmodule("demo-submodule", "demo-submodule-2");
     String afterCommit = repo.commit("Move demo-submodule to demo-submodule-2");
 
     Files.createFile(repo.getDir().resolve("demo-submodule-2").resolve("untracked-file"));
@@ -542,32 +541,71 @@ public void addDependentTargetInSubmodule() throws Exception {
             Set.of("//demo-submodule:submodule_simple"),
             Set.of());
   }
-//
-//  @Test
-//  public void changeSubmodulePath() throws Exception {
-//    doTest(
-//        Commits.SUBMODULE_ADD_DEPENDENT_ON_SIMPLE_JAVA_LIBRARY,
-//        Commits.SUBMODULE_CHANGE_DIRECTORY,
-//        Set.of("//demo-submodule-2:submodule_simple"));
-//
-//    assertThat(
-//        "The old submodule directory should not exist anymore",
-//        not(Files.exists(testDir.resolve("demo-submodule"))));
-//
-//    assertThat(
-//        "The moved submodule should now be present with its README.md but isn't",
-//        Files.exists(testDir.resolve("demo-submodule-2").resolve("README.md")));
-//  }
-//
-//  @Test
-//  public void deleteSubmodule() throws Exception {
-//    doTest(Commits.SUBMODULE_CHANGE_DIRECTORY, Commits.SUBMODULE_DELETE_SUBMODULE, Set.of());
-//
-//    assertThat(
-//        "The old submodule directory should not exist anymore",
-//        not(Files.exists(testDir.resolve("demo-submodule-2"))));
-//  }
-//
+
+  @Test
+  public void targetDeterminationShouldLeaveTheMovedSubmoduleBehind() throws Exception {
+    TestRepo repo = TestRepo.create(testDir);
+
+    repo.replaceWithContentsFrom(Commits.SIMPLE_JAVA_LIBRARY_TARGETS);
+    repo.commit("Initial commit");
+
+    TestRepo submodule = TestRepo.create(tempDir.newFolder("submodule").toPath());
+    submodule.replaceWithContentsFrom(Commits.EMPTY_SUBMODULE);
+    submodule.commit("Create empty submodule");
+
+    TestRepo submoduleWithinRepo = repo.addSubModule(submodule, "demo-submodule");
+
+    // Update the submodule
+    submodule.replaceWithContentsFrom(Commits.ADD_DEPENDENT_ON_SIMPLE_JAVA_LIBRARY);
+    submodule.commit("Update submodule");
+
+    // Now update the reference in the main repo
+    submoduleWithinRepo.pull();
+    String beforeCommit = repo.commit("Add dependent targets in submodule", "demo-submodule");
+
+    repo.moveSubmodule("demo-submodule", "demo-submodule-2");
+    String afterCommit = repo.commit("Move demo-submodule to demo-submodule-2");
+
+    assertTargetDeterminatorRun(
+            beforeCommit,
+            afterCommit,
+            Set.of("//demo-submodule-2:submodule_simple"),
+            Set.of());
+
+    assertThat(
+        "The old submodule directory should not exist anymore",
+        not(Files.exists(testDir.resolve("demo-submodule"))));
+
+    assertThat(
+        "The moved submodule should now be present with its README.md but isn't",
+        Files.exists(testDir.resolve("demo-submodule-2").resolve("README.md")));
+  }
+
+  @Test
+  public void targetDeterminationDoesNotLeaveDeletedSubmoduleDirectoriesBehind() throws Exception {
+    TestRepo repo = TestRepo.create(testDir);
+
+    repo.replaceWithContentsFrom(Commits.SIMPLE_JAVA_LIBRARY_TARGETS);
+    repo.commit("Initial commit");
+
+    TestRepo submodule = TestRepo.create(tempDir.newFolder("submodule").toPath());
+    submodule.replaceWithContentsFrom(Commits.EMPTY_SUBMODULE);
+    submodule.commit("Create empty submodule");
+
+    repo.addSubModule(submodule, "demo-submodule");
+    String before = repo.commit("Add a submodule");
+
+    // Now delete the submodule
+    repo.removeSubmodule("demo-submodule");
+    String after = repo.commit("Delete submodule");
+
+    assertTargetDeterminatorRun(before, after, Set.of(), Set.of());
+
+    assertThat(
+        "The old submodule directory should not exist anymore",
+        not(Files.exists(testDir.resolve("demo-submodule-2"))));
+  }
+
   @Test
   public void testRelativeRevisions() throws Exception {
     TestRepo repo = TestRepo.create(testDir);
@@ -727,8 +765,10 @@ private void assertTargetDeterminatorRun(
     if (supportsIgnoredUnstagedFiles()) {
       try {
         Path ignoredDirectory = testDir.resolve(ignoredDirectoryName);
-        Files.createDirectory(ignoredDirectory);
-        Files.createFile(ignoredDirectory.resolve(ignoredFileName));
+        if (!Files.exists(ignoredDirectory)) {
+          Files.createDirectory(ignoredDirectory);
+          Files.createFile(ignoredDirectory.resolve(ignoredFileName));
+        }
       } catch (IOException e) {
         throw new UncheckedIOException(e);
       }