test: assert the v1 import never writes to plugins/SetHomes/

.github/workflows/issue-status.yml

@@ -29,18 +29,31 @@ jobs:
       - name: Check out
         uses: actions/checkout@v4
 
-      - name: Move the issue to In progress
+      - name: Move the issue along
         env:
           BRANCH: ${{ github.ref_name }}
         run: |
           set -uo pipefail
+          source scripts/issue-status.sh
+
           issue="$(printf '%s' "$BRANCH" | sed -nE 's/^issue-([0-9]+)-.*/\1/p')"
           if [ -z "$issue" ]; then
             echo "Branch $BRANCH does not name an issue - nothing to do."
             exit 0
           fi
-          # Todo or unset only, so a later push cannot pull it out of In review.
-          bash scripts/set-issue-status.sh "$issue" "In progress" Todo unset
+
+          # Pushing to a branch that is already under review, by merging the
+          # base branch in for example, must not report it as in progress.
+          # synchronize is not a trigger type, so this job is the only thing
+          # that runs on such a push.
+          if [ "$(gh pr list --head "$BRANCH" --state open --json number --jq 'length')" = "0" ]; then
+            state="none"
+          else
+            state="open-pr"
+          fi
+
+          mapfile -t plan < <(push_plan "$state")
+          bash scripts/set-issue-status.sh "$issue" "${plan[0]}" "${plan[@]:1}"
 
   pull-request:
     # Fork runs get no secrets, so GH_TOKEN would be empty and every external

scripts/issue-status.sh

@@ -28,3 +28,18 @@ pr_numbers_from_log() {
     | awk '!seen[$0]++'
   return 0
 }
+
+# What a push should do, given `open-pr` when the branch already has an open
+# pull request and anything else when it does not. Prints the target status
+# first, then the states it may advance from, one per line.
+#
+# A branch under review must not be reported as work in progress: merging the
+# base branch in is a push like any other. Neither plan names Ready for release
+# or Done, so a push can never pull a shipped issue backwards.
+push_plan() {
+  if [ "${1:-}" = "open-pr" ]; then
+    printf 'In review\nTodo\nunset\nIn progress\n'
+  else
+    printf 'In progress\nTodo\nunset\n'
+  fi
+}

scripts/test-issue-status.sh

@@ -163,6 +163,50 @@ Merge pull request #31 from Blockframe-Studios/fix/bukkitdev-metadata-semicolon'
   assert_equals "deduped" "31" "$(prs_of "$log")"
 }
 
+# Joins a push plan with commas so an expectation reads as one string.
+plan_of() {
+  push_plan "$1" | paste -sd, -
+}
+
+# -- what a push should aim for --
+
+test_a_branch_under_review_aims_for_in_review() {
+  assert_equals "target under review" "In review" "$(push_plan open-pr | head -1)"
+}
+
+test_a_branch_under_review_may_advance_from_in_progress() {
+  assert_equals "allowed under review" "In review,Todo,unset,In progress" \
+    "$(plan_of open-pr)"
+}
+
+test_a_branch_with_no_pull_request_aims_for_in_progress() {
+  assert_equals "target with no pull request" "In progress" \
+    "$(push_plan none | head -1)"
+}
+
+test_a_branch_with_no_pull_request_cannot_leave_in_progress() {
+  assert_equals "allowed with no pull request" "In progress,Todo,unset" \
+    "$(plan_of none)"
+}
+
+# Neither plan lists Ready for release or Done, so a push can never pull an
+# issue back out of either.
+test_neither_plan_can_disturb_a_shipped_issue() {
+  local plan
+  for arg in open-pr none; do
+    plan="$(plan_of "$arg")"
+    # Asserted first, so an empty plan cannot pass this test by matching nothing.
+    if [ -z "$plan" ]; then
+      fail "$arg plan is empty"
+      continue
+    fi
+    case "$plan" in
+      *"Ready for release"*|*Done*) fail "$arg plan must not list a shipped state" ;;
+      *) pass "$arg plan leaves shipped states alone" ;;
+    esac
+  done
+}
+
 test_several_merges_keep_first_appearance_order() {
   local log
   log='Merge pull request #58 from Blockframe-Studios/issue-53-refuse-alongside-v1
@@ -192,6 +236,12 @@ test_an_empty_range_is_empty_and_clean
 test_repeated_numbers_collapse
 test_several_merges_keep_first_appearance_order
 
+test_a_branch_under_review_aims_for_in_review
+test_a_branch_under_review_may_advance_from_in_progress
+test_a_branch_with_no_pull_request_aims_for_in_progress
+test_a_branch_with_no_pull_request_cannot_leave_in_progress
+test_neither_plan_can_disturb_a_shipped_issue
+
 printf '\n%d passed, %d failed\n' "$PASS" "$FAIL"
 if [ "$FAIL" -gt 0 ]; then
   exit 1

src/test/java/com/samleighton/sethomestwo/importers/SetHomesV1SourceUntouchedTest.java

@@ -0,0 +1,142 @@
+package com.samleighton.sethomestwo.importers;
+
+import com.samleighton.sethomestwo.support.ServerTestBase;
+import org.bukkit.configuration.file.YamlConfiguration;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.List;
+import java.util.Map;
+import java.util.TreeMap;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * The migration is reversible only because v2 never writes, renames or deletes
+ * anything under plugins/SetHomes/. Put the old jar back and the server is as
+ * it was. Nothing else in the suite holds the importer to that.
+ */
+class SetHomesV1SourceUntouchedTest extends ServerTestBase {
+
+    private final SetHomesV1Importer importer = new SetHomesV1Importer();
+
+    private File v1Dir;
+
+    @BeforeEach
+    void writeV1Source() throws IOException {
+        v1Dir = new File(plugin.getDataFolder().getParentFile(), "SetHomes");
+        if (!v1Dir.isDirectory() && !v1Dir.mkdirs())
+            throw new IllegalStateException("could not create " + v1Dir);
+
+        writeHomes();
+        writeBlacklist();
+        writeConfig();
+    }
+
+    @Test
+    void aDryRunLeavesEveryV1FileExactlyAsItWas() throws IOException {
+        Map<String, String> before = fingerprint(v1Dir);
+
+        importer.run(true);
+
+        assertEquals(before, fingerprint(v1Dir),
+                "a preview that writes to v1's folder is not a preview");
+    }
+
+    @Test
+    void aConfirmedImportLeavesEveryV1FileExactlyAsItWas() throws IOException {
+        Map<String, String> before = fingerprint(v1Dir);
+
+        ImportReport report = importer.run(false);
+
+        // Without this the test would also pass for an importer that did nothing.
+        assertTrue(report.imported > 0, "the import should have brought homes across");
+        assertTrue(report.renamed > 0, "the fixture holds a case-only duplicate");
+        assertTrue(report.skippedWorldMissing > 0, "the fixture holds a home in a missing world");
+
+        assertEquals(before, fingerprint(v1Dir),
+                "rolling back to v1 depends on its files being untouched");
+    }
+
+    /**
+     * Path to SHA-256 for every file under the directory. Comparing the whole
+     * map catches an edited file, a new one and a deleted one alike.
+     */
+    private Map<String, String> fingerprint(File dir) throws IOException {
+        Map<String, String> digests = new TreeMap<>();
+        File[] entries = dir.listFiles();
+        if (entries == null) throw new IllegalStateException("not a directory: " + dir);
+
+        for (File entry : entries) {
+            if (entry.isDirectory()) {
+                // Recorded in its own right, or an added empty directory would
+                // contribute no entries and slip through unnoticed.
+                digests.put(entry.getName() + "/", "directory");
+                fingerprint(entry).forEach((path, digest) -> digests.put(entry.getName() + "/" + path, digest));
+            } else {
+                digests.put(entry.getName(), sha256(entry));
+            }
+        }
+        return digests;
+    }
+
+    private String sha256(File file) throws IOException {
+        try {
+            byte[] digest = MessageDigest.getInstance("SHA-256").digest(Files.readAllBytes(file.toPath()));
+            StringBuilder hex = new StringBuilder();
+            for (byte b : digest) hex.append(String.format("%02x", b));
+            return hex.toString();
+        } catch (NoSuchAlgorithmException e) {
+            throw new IllegalStateException("SHA-256 is required of every JVM", e);
+        }
+    }
+
+    /**
+     * The cases that make the importer work hardest, so the test is not proving
+     * read-only behavior on a file the importer barely looks at: a home in a
+     * world that no longer exists, a case-only duplicate name, an unnamed home,
+     * and a player the server has never seen.
+     */
+    private void writeHomes() throws IOException {
+        String owner = UUID.randomUUID().toString();
+        String stranger = UUID.randomUUID().toString();
+
+        YamlConfiguration yaml = new YamlConfiguration();
+        home(yaml, "allNamedHomes." + owner + ".base", "world");
+        home(yaml, "allNamedHomes." + owner + ".Base", "world");
+        home(yaml, "allNamedHomes." + owner + ".nether", "world_nether");
+        home(yaml, "allNamedHomes." + owner + ".plotworld", "creative");
+        home(yaml, "unknownHomes." + stranger, "world");
+        yaml.save(new File(v1Dir, "homes.yml"));
+    }
+
+    private void home(YamlConfiguration yaml, String path, String world) {
+        yaml.set(path + ".world", world);
+        yaml.set(path + ".x", 1.5);
+        yaml.set(path + ".y", 64.0);
+        yaml.set(path + ".z", -2.5);
+        yaml.set(path + ".pitch", 12.0);
+        yaml.set(path + ".yaw", 45.0);
+    }
+
+    private void writeBlacklist() throws IOException {
+        YamlConfiguration yaml = new YamlConfiguration();
+        yaml.set("blacklisted_worlds", List.of("world_the_end"));
+        yaml.save(new File(v1Dir, "world_blacklist.yml"));
+    }
+
+    private void writeConfig() throws IOException {
+        YamlConfiguration yaml = new YamlConfiguration();
+        yaml.set("tp-delay", 5);
+        yaml.set("tp-cancelOnMove", true);
+        yaml.set("max-homes.default", 3);
+        yaml.save(new File(v1Dir, "config.yml"));
+    }
+}