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 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")); + } +}