From 8db2e3c02ab500c5e56b5bd90e8ab280898a5d89 Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Wed, 23 Sep 2026 23:52:46 +0800 Subject: [PATCH] fix(installer): make completion summary dry-run aware MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A TTY `install --dry-run` printed the real-run summary after the preview: "Installation Complete!", "OpenBoot has successfully configured your Mac.", "What was installed: ...", and "Restart your terminal to apply changes", although nothing was changed. showCompletionFromPlan now hands dry-run plans to a separate summary: "Dry run complete — no changes were made" (or "... finished with errors"), "Would install:" with the package counts, and a next step to re-run without --dry-run. It has no Git line, because whether Git would be configured depends on the existing identity, which the Git step already reports. The real-run summary is unchanged and now pinned line for line by a test. recordReporter also records non-header messages so tests can assert what the summary says. --- internal/installer/applycontext_abort_test.go | 22 +++-- internal/installer/installer.go | 33 +++++++ internal/installer/installer_extra_test.go | 86 +++++++++++++++++++ 3 files changed, 132 insertions(+), 9 deletions(-) diff --git a/internal/installer/applycontext_abort_test.go b/internal/installer/applycontext_abort_test.go index b780a63..9722391 100644 --- a/internal/installer/applycontext_abort_test.go +++ b/internal/installer/applycontext_abort_test.go @@ -12,15 +12,19 @@ import ( ) // recordReporter captures Header calls so a test can assert which install -// phases ApplyContext actually entered. -type recordReporter struct{ headers []string } - -func (r *recordReporter) Header(msg string) { r.headers = append(r.headers, msg) } -func (r *recordReporter) Info(string) {} -func (r *recordReporter) Success(string) {} -func (r *recordReporter) Warn(string) {} -func (r *recordReporter) Error(string) {} -func (r *recordReporter) Muted(string) {} +// phases ApplyContext actually entered, and every other message in lines so a +// test can assert what the user was told. +type recordReporter struct { + headers []string + lines []string +} + +func (r *recordReporter) Header(msg string) { r.headers = append(r.headers, msg) } +func (r *recordReporter) Info(msg string) { r.lines = append(r.lines, msg) } +func (r *recordReporter) Success(msg string) { r.lines = append(r.lines, msg) } +func (r *recordReporter) Warn(msg string) { r.lines = append(r.lines, msg) } +func (r *recordReporter) Error(msg string) { r.lines = append(r.lines, msg) } +func (r *recordReporter) Muted(msg string) { r.lines = append(r.lines, msg) } // A cancelled context must stop ApplyContext before the config steps run, so a // ctrl+c abort doesn't keep symlinking dotfiles / rewriting macOS defaults after diff --git a/internal/installer/installer.go b/internal/installer/installer.go index 00b674b..3beb8c2 100644 --- a/internal/installer/installer.go +++ b/internal/installer/installer.go @@ -216,6 +216,10 @@ func abortWith(softErrs []error, cause error) error { } func showCompletionFromPlan(plan InstallPlan, r Reporter, errCount int) { + if plan.DryRun { + showDryRunCompletionFromPlan(plan, r, errCount) + return + } ui.Println() if errCount > 0 { r.Header("Installation finished with errors") @@ -249,6 +253,35 @@ func showCompletionFromPlan(plan InstallPlan, r Reporter, errCount int) { ui.Println() } +// showDryRunCompletionFromPlan is the summary for a --dry-run. Nothing was +// changed, so it must not claim anything was installed or configured: it says +// what a real run would install and how to start one. Git has no line here — +// whether it would be configured depends on the existing identity, and the Git +// step above has already said which way it goes. +func showDryRunCompletionFromPlan(plan InstallPlan, r Reporter, errCount int) { + ui.Println() + if errCount > 0 { + r.Header("Dry run finished with errors — no changes were made") + ui.Println() + r.Warn(fmt.Sprintf("%d step(s) had errors — check the output above for details.", errCount)) + } else { + r.Header("Dry run complete — no changes were made") + } + ui.Println() + + r.Info("Would install:") + r.Info(fmt.Sprintf(" - %d CLI packages", len(plan.Formulae))) + r.Info(fmt.Sprintf(" - %d GUI applications", len(plan.Casks))) + if len(plan.Npm) > 0 { + r.Info(fmt.Sprintf(" - %d npm global packages", len(plan.Npm))) + } + ui.Println() + + r.Info("Next steps:") + r.Info(" - Run the same command without --dry-run to apply these changes") + ui.Println() +} + func showScreenRecordingReminderFromPlan(plan InstallPlan) { if plan.DryRun || plan.Silent { return diff --git a/internal/installer/installer_extra_test.go b/internal/installer/installer_extra_test.go index 14213c5..03fd875 100644 --- a/internal/installer/installer_extra_test.go +++ b/internal/installer/installer_extra_test.go @@ -1,6 +1,7 @@ package installer import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -133,6 +134,91 @@ func TestShowCompletionFromPlan_WithNpm(t *testing.T) { }) } +// A real run's summary is pinned line for line, so making the dry-run summary +// honest can't quietly reword it. +func TestShowCompletionFromPlan_RealRunOutput(t *testing.T) { + tests := []struct { + name string + errCount int + wantHeaders []string + wantStatus string + }{ + {"clean", 0, []string{"Installation Complete!"}, "OpenBoot has successfully configured your Mac."}, + {"with errors", 2, []string{"Installation finished with errors"}, "2 step(s) had errors — check the output above for details."}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + plan := InstallPlan{ + Silent: true, // keep the screen-recording reminder out of the test + Formulae: []string{"git", "curl"}, + Casks: []string{"firefox"}, + Npm: []string{"typescript"}, + } + rr := &recordReporter{} + showCompletionFromPlan(plan, rr, tt.errCount) + + assert.Equal(t, tt.wantHeaders, rr.headers) + assert.Equal(t, []string{ + tt.wantStatus, + "What was installed:", + " - Git configured with your identity", + " - 2 CLI packages", + " - 1 GUI applications", + " - 1 npm global packages", + "Next steps:", + " - Restart your terminal to apply changes", + " - Run 'brew doctor' to verify Homebrew health", + }, rr.lines) + }) + } +} + +// A --dry-run changes nothing, so its summary must not say it did — no +// "Installation Complete!", no "What was installed:", no "restart your +// terminal to apply changes". It reports what a real run would install. +func TestShowCompletionFromPlan_DryRunDoesNotClaimChanges(t *testing.T) { + tests := []struct { + name string + errCount int + wantHeader string + wantWarn string + }{ + {"clean", 0, "Dry run complete — no changes were made", ""}, + {"with errors", 2, "Dry run finished with errors — no changes were made", "2 step(s) had errors — check the output above for details."}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + plan := InstallPlan{ + DryRun: true, + Formulae: []string{"git", "curl"}, + Casks: []string{"firefox"}, + Npm: []string{"typescript"}, + } + rr := &recordReporter{} + showCompletionFromPlan(plan, rr, tt.errCount) + + assert.Equal(t, []string{tt.wantHeader}, rr.headers) + out := strings.Join(rr.lines, "\n") + assert.Contains(t, out, "Would install:\n - 2 CLI packages\n - 1 GUI applications\n - 1 npm global packages") + assert.Contains(t, out, "Run the same command without --dry-run to apply these changes") + if tt.wantWarn != "" { + assert.Contains(t, out, tt.wantWarn) + } + for _, claim := range []string{ + "What was installed:", + "successfully configured", + "Git configured", + "Restart your terminal", + "brew doctor", + } { + assert.NotContains(t, out, claim) + } + }) + } +} + // --------------------------------------------------------------------------- // Apply (happy path with NopReporter through DryRun) // ---------------------------------------------------------------------------