diff --git a/.github/workflows/auto-format.yml b/.github/workflows/auto-format.yml index 91cddeebd..5057d1135 100644 --- a/.github/workflows/auto-format.yml +++ b/.github/workflows/auto-format.yml @@ -109,14 +109,23 @@ jobs: printf 'Formatting %s file(s):\n' "${#format_files[@]}" printf ' %s\n' "${format_files[@]}" nix run .#format -- "${format_files[@]}" - if git diff --quiet; then + git config user.name "github-actions[bot]" + git config user.email "41898282+github-actions[bot]@users.noreply.github.com" + # Stage every tracked change treefmt actually made, not just + # the listed files: formatters rewrite companions of listed + # files (rustfmt reformats out-of-line mod children of a + # listed parent), and staging only the listed files either + # silently dropped those companion changes or failed the + # commit below with "no changes added to commit" (#5014). + # The checkout was clean before treefmt ran, so every + # tracked modification present now is treefmt's. Decide + # "nothing to commit" from the staged diff instead. + git add -u + if git diff --cached --quiet; then echo "treefmt made no changes." record_success exit 0 fi - git config user.name "github-actions[bot]" - git config user.email "41898282+github-actions[bot]@users.noreply.github.com" - git add -- "${format_files[@]}" git commit -m "style(treefmt): auto-format on main" pushed=0 for attempt in 1 2; do diff --git a/scripts/tests/test_auto_format_workflow.py b/scripts/tests/test_auto_format_workflow.py index 345102bb2..08d080005 100644 --- a/scripts/tests/test_auto_format_workflow.py +++ b/scripts/tests/test_auto_format_workflow.py @@ -2,6 +2,7 @@ import os from pathlib import Path +import shlex import subprocess import tempfile import unittest @@ -11,10 +12,17 @@ class AutoFormatWorkflowTest(unittest.TestCase): - def _run_scheduled_pass(self, files, last_success_sha=None): + def _run_scheduled_pass( + self, files, last_success_sha=None, base_files=None, formatter_edits=None + ): # Run the workflow's real run block against a repo whose history # contains exactly the given files. `gh` and `nix` are stubbed so # record_success is observable and no network or Nix is touched. + # `base_files` are committed first; pass last_success_sha="base" + # to diff against that commit, so `files` are the only changed + # (listed) files. `formatter_edits` maps path -> content the nix + # stub writes when invoked, simulating changes treefmt makes to + # files that were not listed (e.g. rustfmt mod children). workflow = WORKFLOW.read_text() block = workflow.split( " - name: Format changed files with treefmt and commit any fixes\n", 1 @@ -28,45 +36,73 @@ def _run_scheduled_pass(self, files, last_success_sha=None): root = Path(temp) repo = root / "repo" repo.mkdir() - subprocess.run(["git", "init", "-q", str(repo)], check=True) - def git(*args): + def git(*args, **kwargs): subprocess.run( - ["git", *args], cwd=repo, check=True, capture_output=True + ["git", *args], cwd=repo, check=True, capture_output=True, **kwargs ) + subprocess.run(["git", "init", "-q", str(repo)], check=True) git("config", "user.name", "Test") git("config", "user.email", "test@example.com") - for name, content in files.items(): - path = repo / name - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(content) - git("add", ".") - git("commit", "-qm", "initial") - head = subprocess.run( - ["git", "rev-parse", "HEAD"], - cwd=repo, - check=True, - capture_output=True, - text=True, - ).stdout.strip() + + def commit_files(contents, message): + for name, content in contents.items(): + path = repo / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) + git("add", ".") + git("commit", "-qm", message) + return subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=repo, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + base_head = None + if base_files: + base_head = commit_files(base_files, "base") + head = commit_files(files, "initial") + + # A bare origin so the write pass can really push, and the + # test can inspect the pushed commit afterwards. + origin = root / "origin.git" + subprocess.run( + ["git", "init", "-q", "--bare", str(origin)], check=True + ) + git("remote", "add", "origin", str(origin)) + git("push", "-q", "origin", "HEAD:main") bindir = root / "bin" bindir.mkdir() nix = bindir / "nix" - nix.write_text('#!/bin/sh\nprintf "%s\\n" "$@" > "$NIX_ARGS"\n') + nix_lines = [ + "#!/bin/sh\n", + 'printf "%s\\n" "$@" > "$NIX_ARGS"\n', + ] + for name, content in (formatter_edits or {}).items(): + parent = str(Path(name).parent) + if parent != ".": + nix_lines.append(f"mkdir -p {shlex.quote(parent)}\n") + nix_lines.append( + f"printf '%s' {shlex.quote(content)} > {shlex.quote(name)}\n" + ) + nix.write_text("".join(nix_lines)) nix.chmod(0o755) args = root / "nix-args" recorded = root / "recorded-sha" # The workflow updates the variable with the HEAD value; the - # stub records the value it was asked to store. + # stub records whatever value it was asked to store. gh = bindir / "gh" gh.write_text( "#!/bin/sh\n" "while [ $# -gt 1 ]; do\n" - ' if [ "$1" = "-f" ] && [ "$2" = "value=$GH_RECORD_VALUE" ]; then\n' - ' printf "%s\\n" "$GH_RECORD_VALUE" > "$GH_RECORD_FILE"\n' - " exit 0\n" + ' if [ "$1" = "-f" ]; then\n' + ' case "$2" in\n' + ' value=*) printf "%s\\n" "${2#value=}" > "$GH_RECORD_FILE"; exit 0;;\n' + " esac\n" " fi\n" " shift\n" "done\n" @@ -78,10 +114,14 @@ def git(*args): "PATH": f"{bindir}:{os.environ['PATH']}", "GITHUB_EVENT_NAME": "schedule", "GITHUB_REPOSITORY": "developing-today/code", + "REPO_AUTOMATION_TOKEN": "test-token", + "GH_TOKEN": "test-token", "NIX_ARGS": str(args), "GH_RECORD_FILE": str(recorded), "GH_RECORD_VALUE": head, } + if last_success_sha == "base": + last_success_sha = base_head if last_success_sha is not None: env["LAST_SUCCESS_SHA"] = last_success_sha result = subprocess.run( @@ -93,19 +133,40 @@ def git(*args): ) nix_args = args.read_text().splitlines() if args.exists() else None recorded_sha = recorded.read_text().strip() if recorded.exists() else None - return result, nix_args, recorded_sha, head + pushed_head = subprocess.run( + ["git", "--git-dir", str(origin), "rev-parse", "main"], + check=True, + capture_output=True, + text=True, + ).stdout.strip() + pushed_tree = {} + names = subprocess.run( + ["git", "--git-dir", str(origin), "ls-tree", "-r", "--name-only", "main"], + check=True, + capture_output=True, + text=True, + ).stdout.splitlines() + for name in names: + pushed_tree[name] = subprocess.run( + ["git", "--git-dir", str(origin), "show", f"main:{name}"], + check=True, + capture_output=True, + text=True, + ).stdout + return result, nix_args, recorded_sha, head, pushed_tree, pushed_head def test_deleted_final_path_does_not_abort_scheduled_filter(self): - result, nix_args, _, _ = self._run_scheduled_pass( + result, nix_args, _, _, _, _ = self._run_scheduled_pass( {"a-existing": "keep\n", "z-deleted": "delete\n"} ) self.assertEqual(result.returncode, 0, result.stderr) self.assertIn("a-existing", result.stdout) - def test_workflow_files_do_not_block_unrelated_formatting(self): - # Mixed change: .yml and .yaml workflow files are skipped and - # logged; the unrelated file is still formatted (#3118). - result, nix_args, _, _ = self._run_scheduled_pass( + def test_workflow_files_are_formatted_like_everything_else(self): + # Under the REPO_AUTOMATION_TOKEN PAT arrangement the bot can + # push workflow files, so nothing is excluded from the write + # pass (#3118, superseding the old GITHUB_TOKEN skip behavior). + result, nix_args, _, _, _, _ = self._run_scheduled_pass( { "notes.txt": "keep\n", ".github/workflows/ci.yml": "name: ci\n", @@ -113,36 +174,52 @@ def test_workflow_files_do_not_block_unrelated_formatting(self): } ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertEqual(nix_args, ["run", ".#format", "--", "notes.txt"]) - self.assertIn("Skipping workflow file(s)", result.stdout) - self.assertIn(".github/workflows/ci.yml", result.stdout) - self.assertIn(".github/workflows/check.yaml", result.stdout) - - def test_workflow_only_change_records_success_without_formatting(self): - # Workflow-only change: nothing the bot may push, clean exit, - # and the formatter is never invoked (#3118). The pass must - # record success so the next run can take the fast path. - result, nix_args, recorded_sha, head = self._run_scheduled_pass( + self.assertEqual( + nix_args, + [ + "run", + ".#format", + "--", + ".github/workflows/check.yaml", + ".github/workflows/ci.yml", + "notes.txt", + ], + ) + + def test_workflow_only_change_records_success_after_formatting(self): + # Workflow-only change under the PAT arrangement: the formatter + # runs on the workflow files; with no resulting changes the + # pass records success so the next run can take the fast path. + result, nix_args, recorded_sha, head, _, _ = self._run_scheduled_pass( { ".github/workflows/ci.yml": "name: ci\n", ".github/workflows/check.yaml": "name: check\n", } ) self.assertEqual(result.returncode, 0, result.stderr) - self.assertIsNone(nix_args) - self.assertIn("Only workflow files changed", result.stdout) + self.assertEqual( + nix_args, + [ + "run", + ".#format", + "--", + ".github/workflows/check.yaml", + ".github/workflows/ci.yml", + ], + ) + self.assertIn("treefmt made no changes.", result.stdout) self.assertEqual(recorded_sha, head) self.assertIn("Recorded AUTO_FORMAT_LAST_SUCCESS_SHA", result.stdout) def test_unchanged_head_takes_fast_path_after_workflow_only_pass(self): - # Once the workflow-only pass records HEAD, a scheduled run whose + # Once a workflow-only pass records HEAD, a scheduled run whose # HEAD still equals it exits before Nix is installed or invoked. files = {".github/workflows/ci.yml": "name: ci\n"} - result, _, recorded_sha, head = self._run_scheduled_pass(files) + result, _, recorded_sha, head, _, _ = self._run_scheduled_pass(files) self.assertEqual(result.returncode, 0, result.stderr) self.assertEqual(recorded_sha, head) - result, nix_args, _, _ = self._run_scheduled_pass( + result, nix_args, _, _, _, _ = self._run_scheduled_pass( files, last_success_sha=recorded_sha ) self.assertEqual(result.returncode, 0, result.stderr) @@ -152,6 +229,30 @@ def test_unchanged_head_takes_fast_path_after_workflow_only_pass(self): result.stdout, ) + def test_companion_change_is_committed_and_pushed(self): + # Regression test for #5014: the listed file (parent.rs) is + # already formatted, but the formatter rewrites an unlisted + # tracked companion (child.rs, an out-of-line mod child, as + # rustfmt does). The run must exit 0 and the companion change + # must be in the pushed commit — not stranded, and not failing + # the commit with "no changes added to commit". + result, nix_args, recorded_sha, head, pushed_tree, pushed_head = ( + self._run_scheduled_pass( + {"parent.rs": "mod child;\n\npub fn parent() {}\n"}, + last_success_sha="base", + base_files={"child.rs": "pub fn child( ){}\n"}, + formatter_edits={"child.rs": "pub fn child() {}\n"}, + ) + ) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertEqual(nix_args, ["run", ".#format", "--", "parent.rs"]) + self.assertNotEqual(pushed_head, head) + self.assertEqual(pushed_tree["child.rs"], "pub fn child() {}\n") + self.assertEqual( + pushed_tree["parent.rs"], "mod child;\n\npub fn parent() {}\n" + ) + self.assertEqual(recorded_sha, pushed_head) + if __name__ == "__main__": unittest.main()