From b470419bd2b03bdd669d2f8ed83ced65a6fa710d Mon Sep 17 00:00:00 2001 From: milanmalhotra Date: Mon, 17 Aug 2026 19:37:21 -0400 Subject: [PATCH 1/2] test: assert the v1 import never writes to plugins/SetHomes/ 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. That guarantee is why taking the name SetHomes was rejected, but nothing in the suite held the importer to it. Fingerprints every path under the v1 folder as SHA-256 before and after, for the dry run and for confirm, and compares the whole map so an edit, an addition and a deletion all fail alike. Directories are recorded in their own right, or an added empty folder would contribute no entries and pass unnoticed. No changeset: src/test is exempt from the gate and nothing here ships. --- .../SetHomesV1SourceUntouchedTest.java | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 src/test/java/com/samleighton/sethomestwo/importers/SetHomesV1SourceUntouchedTest.java diff --git a/src/test/java/com/samleighton/sethomestwo/importers/SetHomesV1SourceUntouchedTest.java b/src/test/java/com/samleighton/sethomestwo/importers/SetHomesV1SourceUntouchedTest.java new file mode 100644 index 0000000..62a3e3f --- /dev/null +++ b/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 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 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 fingerprint(File dir) throws IOException { + Map 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")); + } +} From 5db506f300ca389d1c2f6197716ddf48e4c37b6e Mon Sep 17 00:00:00 2001 From: milanmalhotra Date: Mon, 17 Aug 2026 21:45:03 -0400 Subject: [PATCH 2/2] fix: keep a branch under review from falling back to In progress The push job chose its target from the branch name alone, so merging the base branch into a branch that already had an open pull request reported the issue as in progress. The Todo and unset guard could not help: it stops an issue being pulled out of In review, but an issue that never reached In review is not protected by it. The job now asks whether the branch has an open pull request and aims for In review when it does, advancing from In progress as well as Todo and unset. Neither plan names Ready for release or Done, so a push still cannot disturb a shipped issue. The decision lives in push_plan so it is covered by the suite. --- .github/workflows/issue-status.yml | 19 ++++++++++-- scripts/issue-status.sh | 15 +++++++++ scripts/test-issue-status.sh | 50 ++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/.github/workflows/issue-status.yml b/.github/workflows/issue-status.yml index 09c2862..96a058b 100644 --- a/.github/workflows/issue-status.yml +++ b/.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 diff --git a/scripts/issue-status.sh b/scripts/issue-status.sh index dfef119..6c3be92 100644 --- a/scripts/issue-status.sh +++ b/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 +} diff --git a/scripts/test-issue-status.sh b/scripts/test-issue-status.sh index 4328d49..51dfb47 100644 --- a/scripts/test-issue-status.sh +++ b/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