diff --git a/internal/cli/packages.go b/internal/cli/packages.go index 1a625f6..cce551b 100644 --- a/internal/cli/packages.go +++ b/internal/cli/packages.go @@ -160,12 +160,52 @@ func runRestore(cmd *cobra.Command, args []string) error { } } + // writeReport persists a MigrationReport so the user has a record + // of what migrated. We only write when at least one package was + // attempted — skipping the write on an empty results slice avoids + // producing a useless "0 packages attempted" file for fresh installs + // with no globals. The snapshot's recorded Manager / NodeVersion + // are preferred over the CLI's inferred values when present, so the + // --from branch (which has no CLI-side manager argument) gets an + // accurate report. + writeReport := func(snapshot packages.SnapshotData, fallbackMgr, fallbackFrom string, results []packages.PackageResult) { + if len(results) == 0 { + return + } + mgr := snapshot.Manager + if mgr == "" { + mgr = fallbackMgr + } + fromVer := snapshot.NodeVersion + if fromVer == "" { + fromVer = fallbackFrom + } + report := packages.NewMigrationReport(mgr, fromVer, "") + for _, r := range results { + report.AddResult(r) + } + if err := report.Save(); err != nil { + cmd.Printf("Warning: failed to write migration report: %v\n", err) + return + } + if p, perr := report.Path(); perr == nil { + cmd.Printf("Migration report: %s\n", p) + } + } + // --from branch: read the path straight off disk, no manager or // version parsing required. if fromPath != "" { - if err := packages.RestoreFromSnapshot(ctx, fromPath); err != nil { + outcome, err := packages.RestoreFromSnapshot(ctx, fromPath) + if err != nil { + // Even on partial failure, persist whatever results we + // collected so the user has a file to inspect — the + // returned slice still contains entries for every package + // attempted, including failures (status="failed"). + writeReport(outcome.Snapshot, "", "", outcome.Results) return fmt.Errorf("restore failed: %w", err) } + writeReport(outcome.Snapshot, "", "", outcome.Results) clearSentinel() cmd.Printf("Restored packages from %s\n", fromPath) return nil @@ -192,9 +232,15 @@ func runRestore(cmd *cobra.Command, args []string) error { return fmt.Errorf("invalid version: %w", err) } - if err := packages.Restore(ctx, managerName, *v); err != nil { + outcome, err := packages.Restore(ctx, managerName, *v) + if err != nil { + // Partial failure: persist a report (with the manager name + // and source version so the user can correlate) so they know + // which packages to retry. + writeReport(outcome.Snapshot, managerName, versionStr, outcome.Results) return fmt.Errorf("restore failed: %w", err) } + writeReport(outcome.Snapshot, managerName, versionStr, outcome.Results) clearSentinel() cmd.Printf("Restored packages for %s %s\n", managerName, versionStr) diff --git a/internal/cli/upgrade.go b/internal/cli/upgrade.go index babbfaa..cd494a3 100644 --- a/internal/cli/upgrade.go +++ b/internal/cli/upgrade.go @@ -96,6 +96,13 @@ func runUpgrade(cmd *cobra.Command, args []string) error { // on disk so a follow-up `nodeup packages restore --from ` // (the command printed by PersistentPreRunE's hint) can still find // the resume breadcrumb. See issue #46. + // + // "Failure" here includes PARTIAL failure: if one or more packages + // failed to install but the loop did complete (which it now does — + // see #103), restoreSucceeded stays false. The MigrationReport file + // is written either way; the sentinel only clears on a fully-successful + // migration so a follow-up `nodeup packages restore --from ` + // remains available to retry just the failed packages. sentinelArmed := false restoreSucceeded := false defer func() { @@ -306,13 +313,63 @@ func runUpgrade(cmd *cobra.Command, args []string) error { // sentinel already records this snapshot path for // replay-after-interrupt (`nodeup packages restore --from // `); here we read the same path directly. + // + // installPackages now LOOP-ALL (see issue #103): a single failing + // package no longer aborts the rest of the migration. We collect + // the per-package results, persist a MigrationReport to + // /reports/ on both success and partial failure, and + // surface the report path so the user knows what to re-install + // by hand if anything failed. Partial failure keeps the sentinel + // armed (restoreSucceeded stays false) so the user can re-run + // `nodeup packages restore --from ` to retry just the + // failures. if !skipMigrate && len(toInstall) > 0 { if restoreSnapshotPath == "" { cmd.Printf("Warning: no snapshot path available to restore from; skipping package migration\n") - } else if err := packages.RestoreFromSnapshot(cmd.Context(), restoreSnapshotPath); err != nil { - cmd.Printf("Warning: restore failed: %v\n", err) } else { - restoreSucceeded = true + outcome, rerr := packages.RestoreFromSnapshot(cmd.Context(), restoreSnapshotPath) + + // Write a MigrationReport whenever we actually attempted at + // least one package. Skipping the write on empty results + // avoids producing a useless "0 packages attempted" file + // for a fresh install with no globals. On a context + // cancellation, results may be truncated; we still record + // whatever we managed to install, with Interrupted=true so + // the report is self-describing. + if len(outcome.Results) > 0 { + // Prefer the snapshot's recorded manager / from-version + // if present (the upgrade flow wrote the snapshot with + // these fields populated, so this is more accurate than + // the CLI's inferred values). + reportMgr := outcome.Snapshot.Manager + if reportMgr == "" { + reportMgr = m.Name() + } + reportFrom := outcome.Snapshot.NodeVersion + if reportFrom == "" { + reportFrom = oldVersion + } + report := packages.NewMigrationReport(reportMgr, reportFrom, toInstall[len(toInstall)-1].String()) + report.Interrupted = errors.Is(rerr, context.Canceled) || errors.Is(rerr, context.DeadlineExceeded) + for _, res := range outcome.Results { + report.AddResult(res) + } + if saveErr := report.Save(); saveErr != nil { + cmd.Printf("Warning: failed to write migration report: %v\n", saveErr) + } else if reportPath, perr := report.Path(); perr == nil { + if rerr != nil { + cmd.Printf("Migration report: %s (partial — see file for failures)\n", reportPath) + } else { + cmd.Printf("Migration report: %s\n", reportPath) + } + } + } + + if rerr != nil { + cmd.Printf("Warning: restore failed: %v\n", rerr) + } else { + restoreSucceeded = true + } } } diff --git a/internal/packages/report.go b/internal/packages/report.go index cb0c8e7..a5b8c7a 100644 --- a/internal/packages/report.go +++ b/internal/packages/report.go @@ -1,6 +1,8 @@ package packages import ( + "crypto/rand" + "encoding/hex" "encoding/json" "fmt" "os" @@ -27,9 +29,17 @@ type MigrationReport struct { ToVersion string `json:"to_version"` Interrupted bool `json:"interrupted"` Results []PackageResult `json:"results"` + + // tag is the on-disk-filename discriminator; assigned lazily on + // the first Path()/Save() call. Not serialized — it's purely a + // filename concern, not part of the report's content. + tag string } -// NewMigrationReport creates a fresh report for a migration. +// NewMigrationReport creates a fresh report for a migration. The +// returned report has a unique 4-byte hex tag in its Timestamp-derived +// filename (see Path/Save) so two upgrades running within the same +// wall-clock second can't accidentally clobber each other's report. func NewMigrationReport(manager, fromVersion, toVersion string) *MigrationReport { return &MigrationReport{ Timestamp: time.Now(), @@ -39,24 +49,80 @@ func NewMigrationReport(manager, fromVersion, toVersion string) *MigrationReport } } -// Save writes the report to ~/.nodeup/reports/.json. +// Save writes the report to /reports/migration--.json. +// The first call computes a stable 4-byte hex tag and caches it on the +// report; subsequent calls (Path and Save) reuse the same tag, so the +// CLI can call Path() before Save() to print the path and have it +// match the file that Save() actually writes. +// +// The tag uses crypto/rand entropy, so two reports produced within the +// same wall-clock second get distinct filenames. 32 bits of entropy is +// plenty to make accidental collisions vanishingly unlikely across the +// lifetime of a single machine's DataDir (~1 report per upgrade, +// upgrades are minutes-to-hours apart for a real user). +// +// Save is NOT safe to call concurrently on the same report instance: +// the tag is assigned once and shared across Path/Save, and two +// parallel Save calls would race to write the same file. Callers +// don't actually need concurrent saves — a single upgrade writes at +// most one report. func (r *MigrationReport) Save() error { - dir, err := platform.ReportsDir() + path, err := r.Path() if err != nil { return err } - - filename := fmt.Sprintf("migration-%s.json", r.Timestamp.Format("20060102-150405")) - path := filepath.Join(dir, filename) - data, err := json.MarshalIndent(r, "", " ") if err != nil { return fmt.Errorf("marshal report: %w", err) } - return os.WriteFile(path, data, 0o644) } +// Path returns the on-disk path Save() would write to. The first call +// assigns a stable random suffix; subsequent calls return the same +// filename. Exposed so the CLI can print the path to the user without +// re-deriving the timestamp-formatting rule. +// +// Two different MigrationReport instances (i.e., two upgrades) get +// independent random suffixes — Path() called on each one resolves to +// a unique filename even within the same second. +func (r *MigrationReport) Path() (string, error) { + if r.tag == "" { + tag, err := reportTag() + if err != nil { + return "", fmt.Errorf("generate report tag: %w", err) + } + r.tag = tag + } + dir, err := platform.ReportsDir() + if err != nil { + return "", err + } + return filepath.Join(dir, r.filename()), nil +} + +// filename is the basename portion of the on-disk path. Centralized +// so Path() and any future tests stay in lockstep with the format. +func (r *MigrationReport) filename() string { + return fmt.Sprintf("migration-%s-%s.json", r.Timestamp.Format("20060102-150405"), r.tag) +} + +// reportTag returns 4 hex bytes (8 chars) of crypto-random entropy +// sourced from crypto/rand. 32 bits of entropy is plenty to make +// accidental collisions vanishingly unlikely across the lifetime of a +// single machine's DataDir (~1 report per upgrade, upgrades are +// minutes-to-hours apart for a real user). We deliberately do NOT use +// math/rand here — that would couple report-naming to Go's seeded PRNG +// and could produce identical tags in two different processes +// initialized identically. +func reportTag() (string, error) { + var b [4]byte + if _, err := rand.Read(b[:]); err != nil { + return "", err + } + return hex.EncodeToString(b[:]), nil +} + // AddResult appends a package result to the report. func (r *MigrationReport) AddResult(pr PackageResult) { r.Results = append(r.Results, pr) diff --git a/internal/packages/sentinel_test.go b/internal/packages/sentinel_test.go index 7569c6d..6ff92fd 100644 --- a/internal/packages/sentinel_test.go +++ b/internal/packages/sentinel_test.go @@ -380,7 +380,7 @@ func TestRestoreFromSnapshot_ValidFile(t *testing.T) { // With an empty Packages list, RestoreFromSnapshot runs the // install loop zero times and returns nil — no need for a fake // `npm install` binary on PATH. - if err := RestoreFromSnapshot(t.Context(), snapPath); err != nil { + if _, err := RestoreFromSnapshot(t.Context(), snapPath); err != nil { t.Errorf("RestoreFromSnapshot: %v", err) } } @@ -389,7 +389,7 @@ func TestRestoreFromSnapshot_ValidFile(t *testing.T) { // includes both the path they typed AND a "read snapshot" hint so they // don't have to read source to figure out what went wrong. func TestRestoreFromSnapshot_MissingFile(t *testing.T) { - err := RestoreFromSnapshot(t.Context(), "/nonexistent/snapshot.json") + _, err := RestoreFromSnapshot(t.Context(), "/nonexistent/snapshot.json") if err == nil { t.Fatal("expected error for missing snapshot file") } @@ -409,7 +409,7 @@ func TestRestoreFromSnapshot_CorruptFile(t *testing.T) { if err := os.WriteFile(snapPath, []byte("{not valid json"), 0o644); err != nil { t.Fatalf("write: %v", err) } - err := RestoreFromSnapshot(t.Context(), snapPath) + _, err := RestoreFromSnapshot(t.Context(), snapPath) if err == nil { t.Fatal("expected error for corrupt JSON") } diff --git a/internal/packages/snapshot.go b/internal/packages/snapshot.go index ec8ca6d..f539ba3 100644 --- a/internal/packages/snapshot.go +++ b/internal/packages/snapshot.go @@ -13,6 +13,14 @@ import ( "github.com/dipto0321/nodeup/internal/platform" ) +// runShell is the package-level seam used by Restore/RestoreFromSnapshot +// to invoke `npm install -g`. Tests overwrite it to capture arguments +// and return canned output (success or per-package failure) without +// spawning a real subprocess. Production code never reassigns it. +// +// Signature matches platform.RunShell so a direct assignment works. +var runShell = platform.RunShell + // Package represents a globally installed npm package. type Package struct { Name string @@ -120,24 +128,47 @@ func saveSnapshot(s SnapshotData) error { return os.WriteFile(path, data, 0o644) } +// RestoreOutcome bundles the parsed snapshot metadata with the +// per-package install results so callers can build an accurate +// MigrationReport without having to re-parse the snapshot file +// themselves. The Snapshot field is populated even on partial failure +// (it's available the moment we successfully unmarshal the file). +type RestoreOutcome struct { + // Snapshot is the parsed snapshot we replayed. Useful for the + // MigrationReport's Manager / FromVersion fields. + Snapshot SnapshotData + // Results carries one entry per package that installPackages + // attempted (success or failure). On partial failure, every + // attempted package is present — including the ones that failed. + Results []PackageResult +} + // Restore reinstalls packages from a snapshot. -func Restore(ctx context.Context, managerName string, version semver.Version) error { +// +// Returns the parsed snapshot and the per-package results so the +// caller can build a MigrationReport (see internal/packages/report.go). +// When one or more packages failed to install, the returned error +// wraps the per-package failures and the returned slice still +// contains an entry for every package — including the failures — so +// callers can persist the full outcome for the user to inspect. +func Restore(ctx context.Context, managerName string, version semver.Version) (RestoreOutcome, error) { path, err := snapshotPath(managerName, version.String()) if err != nil { - return err + return RestoreOutcome{}, err } data, err := os.ReadFile(path) if err != nil { - return fmt.Errorf("read snapshot: %w", err) + return RestoreOutcome{}, fmt.Errorf("read snapshot: %w", err) } var s SnapshotData if err := json.Unmarshal(data, &s); err != nil { - return fmt.Errorf("parse snapshot: %w", err) + return RestoreOutcome{}, fmt.Errorf("parse snapshot: %w", err) } - return installPackages(ctx, s.Packages) + results, err := installPackages(ctx, s.Packages) + return RestoreOutcome{Snapshot: s, Results: results}, err } // RestoreFromSnapshot reinstalls the packages contained in an arbitrary @@ -154,28 +185,86 @@ func Restore(ctx context.Context, managerName string, version semver.Version) er // // We deliberately use os.ReadFile directly instead of LoadSnapshot so // the path does not have to live under /snapshots. -func RestoreFromSnapshot(ctx context.Context, path string) error { +// +// Returns the parsed snapshot and per-package results, plus a non-nil +// error when at least one package failed. Callers can use the +// snapshot metadata to populate the MigrationReport accurately — +// even on the --from branch where the CLI never saw a manager name +// from the user. +func RestoreFromSnapshot(ctx context.Context, path string) (RestoreOutcome, error) { data, err := os.ReadFile(path) if err != nil { - return fmt.Errorf("read snapshot %s: %w", path, err) + return RestoreOutcome{}, fmt.Errorf("read snapshot %s: %w", path, err) } var s SnapshotData if err := json.Unmarshal(data, &s); err != nil { - return fmt.Errorf("parse snapshot %s: %w", path, err) + return RestoreOutcome{}, fmt.Errorf("parse snapshot %s: %w", path, err) } - return installPackages(ctx, s.Packages) + results, err := installPackages(ctx, s.Packages) + return RestoreOutcome{Snapshot: s, Results: results}, err } -func installPackages(ctx context.Context, pkgs []Package) error { +// installPackages runs `npm install -g ` for each package in pkgs, +// recording a PackageResult per package. It LOOP-ALL — a failure on +// package N does NOT short-circuit packages N+1..M, so a single broken +// or yanked package no longer prevents the rest of the user's globals +// from migrating. Per-package npm failures are reflected in the +// returned results slice as Status="failed"; an aggregate error is +// returned when any package failed so callers can detect partial +// failure and persist a MigrationReport. +// +// Context cancellation still aborts the loop promptly: we check +// ctx.Err() at the top of each iteration. On cancellation, the +// returned results contain whatever we managed to record before the +// signal arrived, and the returned error is the ctx error itself +// (NOT a wrapped "N of M failed" message — the caller can branch on +// errors.Is(err, context.Canceled) for that path). +// +// Note: this intentionally diverges from the previous "return on first +// failure" behavior. That behavior is what issue #46 / #103 called +// out as a silent data-loss bug — restoring 30 globals and losing 27 +// of them because package #3 was renamed. +func installPackages(ctx context.Context, pkgs []Package) ([]PackageResult, error) { + results := make([]PackageResult, 0, len(pkgs)) + var failed int + var firstFailedName string + var firstFailedErr error + for _, pkg := range pkgs { - _, err := platform.RunShell(ctx, "npm", "install", "-g", pkgSpec(pkg)) + // Honor cancellation (Ctrl-C / parent-ctx Done) before each + // shell-out so a single long install doesn't block the abort. + if cerr := ctx.Err(); cerr != nil { + return results, cerr + } + + r := PackageResult{ + Name: pkg.Name, + Status: "ok", + AttemptedVersion: pkg.Version, + } + _, err := runShell(ctx, "npm", "install", "-g", pkgSpec(pkg)) if err != nil { - return fmt.Errorf("install %s: %w", pkg.Name, err) + r.Status = "failed" + r.Error = err.Error() + failed++ + if firstFailedName == "" { + firstFailedName = pkg.Name + firstFailedErr = err + } } + results = append(results, r) + } + + if failed > 0 { + // Wrap the first failure so callers can still errors.Is / errors.As + // on the original; the aggregate "N of M" message gives users the + // headline number at a glance. + return results, fmt.Errorf("%d of %d packages failed to install (first failure: %s): %w", + failed, len(pkgs), firstFailedName, firstFailedErr) } - return nil + return results, nil } func pkgSpec(p Package) string { diff --git a/internal/packages/snapshot_test.go b/internal/packages/snapshot_test.go index 21f2bb1..e50ca2b 100644 --- a/internal/packages/snapshot_test.go +++ b/internal/packages/snapshot_test.go @@ -8,9 +8,12 @@ import ( "os" "path/filepath" "strings" + "sync/atomic" "testing" "github.com/Masterminds/semver/v3" + + "github.com/dipto0321/nodeup/internal/platform" ) // TestSnapshot_StoredVersionKeyMatchesArg verifies the on-disk path @@ -82,10 +85,13 @@ func TestRestore_LooksUpByArgVersion(t *testing.T) { if err != nil { t.Fatalf("parse version: %v", err) } - restoreErr := Restore(context.Background(), "fnm", *v) + outcome, restoreErr := Restore(context.Background(), "fnm", *v) if restoreErr == nil { t.Fatal("expected error for missing snapshot, got nil") } + if len(outcome.Results) != 0 { + t.Errorf("expected no results on missing snapshot, got %d", len(outcome.Results)) + } if !strings.Contains(restoreErr.Error(), "read snapshot") { t.Errorf("error %q does not mention read snapshot failure", restoreErr) } @@ -132,7 +138,292 @@ func TestRestoreFromSnapshot_ReadsSourcePathRegardlessOfNodeVersion(t *testing.T // path-based restore does NOT depend on the version-key in the // filename matching some "new version" expectation — it just // reads what's at the path. - if err := RestoreFromSnapshot(context.Background(), arbitraryPath); err != nil { + outcome, err := RestoreFromSnapshot(context.Background(), arbitraryPath) + if err != nil { t.Errorf("RestoreFromSnapshot: %v", err) } + if len(outcome.Results) != 0 { + t.Errorf("expected no results for empty snapshot, got %d (%+v)", len(outcome.Results), outcome.Results) + } + // The parsed snapshot must round-trip back through RestoreOutcome — + // the CLI uses this to populate MigrationReport metadata even on + // the --from branch where no CLI-side manager was given. + if outcome.Snapshot.Manager != "fnm" || outcome.Snapshot.NodeVersion != "20.10.0" { + t.Errorf("outcome.Snapshot not populated: %+v", outcome.Snapshot) + } +} + +// --- #103: continue-past-failure + report write + ctx abort ------------- + +// withRunShellFake replaces the package-level runShell seam for the +// duration of a test, restoring the original (production) value on +// cleanup. Each test that needs to inject canned success/failure +// behavior calls this once; tests must not run in parallel because +// the seam is package-global. +func withRunShellFake(t *testing.T, fn func(ctx context.Context, name string, args ...string) (*platform.RunResult, error)) { + t.Helper() + orig := runShell + runShell = fn + t.Cleanup(func() { runShell = orig }) +} + +// TestInstallPackages_ContinuesPastFailure is the regression pin for +// issue #103 / #46: a failure on package N must NOT abort packages +// N+1..M. Each per-package outcome (ok vs failed) is recorded in +// the returned results slice; the aggregate error wraps the first +// failure so callers can detect "partial failure" but still have a +// per-package record for the MigrationReport. +func TestInstallPackages_ContinuesPastFailure(t *testing.T) { + pkgs := []Package{ + {Name: "ok-1", Version: "1.0.0"}, + {Name: "broken", Version: "2.0.0"}, // this one fails + {Name: "ok-2", Version: "3.0.0"}, + {Name: "broken-2", Version: "4.0.0"}, // and this one + {Name: "ok-3", Version: "5.0.0"}, + } + + var calls atomic.Int32 + withRunShellFake(t, func(_ context.Context, name string, args ...string) (*platform.RunResult, error) { + // Sanity-check: we only expect `npm install -g ` calls. + if name != "npm" || len(args) < 3 || args[0] != "install" || args[1] != "-g" { + t.Errorf("unexpected shell-out: %s %v", name, args) + } + calls.Add(1) + // Fail any package whose name starts with "broken". pkgSpec + // emits "@", so we split on "@" and inspect the + // name half. Matching by name avoids the prefix-overlap bug + // where "broken" matches inside "broken-2@...". + spec := args[2] + nameOnly := spec + if i := strings.Index(spec, "@"); i >= 0 { + nameOnly = spec[:i] + } + if strings.HasPrefix(nameOnly, "broken") { + return nil, errors.New("404 not found: " + spec) + } + return &platform.RunResult{Stdout: "+ " + spec}, nil + }) + + results, err := installPackages(context.Background(), pkgs) + + // 1. The aggregate error wraps the first failure and reports the count. + if err == nil { + t.Fatal("expected aggregate error for partial failure, got nil") + } + if !strings.Contains(err.Error(), "2 of 5 packages failed") { + t.Errorf("aggregate error %q does not report 2 of 5", err) + } + if !strings.Contains(err.Error(), "broken@2.0.0") { + t.Errorf("aggregate error %q does not name the first failed package", err) + } + + // 2. The first underlying failure is reachable via errors.Is / errors.As + // so callers can branch on it. + if !strings.Contains(err.Error(), "404 not found: broken@2.0.0") { + t.Errorf("aggregate error %q does not wrap the original failure", err) + } + + // 3. ALL packages were attempted — the loop did NOT short-circuit. + if got := calls.Load(); got != 5 { + t.Errorf("expected 5 npm install calls, got %d", got) + } + + // 4. Results carry the right per-package statuses. + if len(results) != 5 { + t.Fatalf("expected 5 results, got %d", len(results)) + } + wantStatuses := []string{"ok", "failed", "ok", "failed", "ok"} + for i, r := range results { + if r.Status != wantStatuses[i] { + t.Errorf("results[%d].Status = %q, want %q (name=%q)", i, r.Status, wantStatuses[i], r.Name) + } + if r.Name != pkgs[i].Name { + t.Errorf("results[%d].Name = %q, want %q", i, r.Name, pkgs[i].Name) + } + if r.Error == "" && r.Status == "failed" { + t.Errorf("results[%d] failed but Error is empty", i) + } + if r.Error != "" && r.Status == "ok" { + t.Errorf("results[%d] ok but Error=%q", i, r.Error) + } + } +} + +// TestInstallPackages_AllSucceed returns no error and a clean results +// slice when every package installs cleanly. Pins the happy-path +// contract that callers rely on to compute "restoreSucceeded". +func TestInstallPackages_AllSucceed(t *testing.T) { + pkgs := []Package{ + {Name: "a", Version: "1.0.0"}, + {Name: "b", Version: "2.0.0"}, + } + withRunShellFake(t, func(_ context.Context, _ string, _ ...string) (*platform.RunResult, error) { + return &platform.RunResult{Stdout: "+ok"}, nil + }) + + results, err := installPackages(context.Background(), pkgs) + if err != nil { + t.Fatalf("expected nil error on all-success, got %v", err) + } + if len(results) != 2 { + t.Fatalf("expected 2 results, got %d", len(results)) + } + for i, r := range results { + if r.Status != "ok" { + t.Errorf("results[%d].Status = %q, want ok", i, r.Status) + } + } +} + +// TestInstallPackages_ContextCancelsMidLoop covers the Ctrl-C path: +// we cancel the ctx before iteration N, and the function MUST return +// whatever results it accumulated so far plus the ctx error (NOT the +// "N of M failed" aggregate). Callers can branch on +// errors.Is(err, context.Canceled) to distinguish user-abort from +// per-package failure. +func TestInstallPackages_ContextCancelsMidLoop(t *testing.T) { + pkgs := []Package{ + {Name: "a", Version: "1.0.0"}, + {Name: "b", Version: "2.0.0"}, + {Name: "c", Version: "3.0.0"}, + {Name: "d", Version: "4.0.0"}, + } + + ctx, cancel := context.WithCancel(context.Background()) + + var calls atomic.Int32 + withRunShellFake(t, func(_ context.Context, _ string, _ ...string) (*platform.RunResult, error) { + n := calls.Add(1) + // Cancel after the second successful install — packages + // 3 and 4 must NOT run. + if n >= 2 { + cancel() + } + return &platform.RunResult{Stdout: "+ok"}, nil + }) + + results, err := installPackages(ctx, pkgs) + + if !errors.Is(err, context.Canceled) { + t.Errorf("expected context.Canceled, got %v", err) + } + // We can't strictly pin "exactly 2" — the cancel races with the + // shell-out — but we MUST not have run all 4. + if got := calls.Load(); got >= 4 { + t.Errorf("expected fewer than 4 shell-outs after cancel, got %d", got) + } + // Results contain whatever we managed before the cancel; status is + // "ok" for each completed install. + for i, r := range results { + if r.Status != "ok" { + t.Errorf("results[%d].Status = %q, want ok", i, r.Status) + } + } +} + +// TestInstallPackages_EmptyList is the trivial boundary: no packages, +// no shell-outs, no error. Pins the "fresh install, nothing to +// migrate" case the upgrade flow relies on for first-time runs. +func TestInstallPackages_EmptyList(t *testing.T) { + withRunShellFake(t, func(_ context.Context, _ string, _ ...string) (*platform.RunResult, error) { + t.Errorf("runShell should not be called for empty pkgs") + return nil, nil + }) + results, err := installPackages(context.Background(), nil) + if err != nil { + t.Errorf("empty pkgs: unexpected error %v", err) + } + if len(results) != 0 { + t.Errorf("empty pkgs: expected no results, got %d", len(results)) + } +} + +// TestMigrationReport_SavePersistsAndPathMatches is the wire-up test +// for #103 part 2: every restore — success or partial failure — must +// write a MigrationReport file to /reports/, and the path +// Save() actually writes to must equal Path() (so the CLI can print +// the same path back to the user). +func TestMigrationReport_SavePersistsAndPathMatches(t *testing.T) { + redirectDataDir(t) + + report := NewMigrationReport("fnm", "20.10.0", "20.11.0") + report.AddResult(PackageResult{Name: "typescript", Status: "ok", AttemptedVersion: "5.0.0"}) + report.AddResult(PackageResult{Name: "broken", Status: "failed", AttemptedVersion: "1.0.0", Error: "ENOENT"}) + + wantPath, err := report.Path() + if err != nil { + t.Fatalf("Path: %v", err) + } + // Path() must be stable — calling it again must return the same + // filename, so the CLI's "report at " message matches the + // file that Save() actually writes. + wantPath2, err := report.Path() + if err != nil { + t.Fatalf("Path #2: %v", err) + } + if wantPath != wantPath2 { + t.Errorf("Path() not stable across calls: %q vs %q", wantPath, wantPath2) + } + if err := report.Save(); err != nil { + t.Fatalf("Save: %v", err) + } + + data, err := os.ReadFile(wantPath) + if err != nil { + t.Fatalf("read back report: %v", err) + } + var roundTrip MigrationReport + if err := json.Unmarshal(data, &roundTrip); err != nil { + t.Fatalf("parse report: %v", err) + } + if roundTrip.Manager != "fnm" || roundTrip.FromVersion != "20.10.0" || roundTrip.ToVersion != "20.11.0" { + t.Errorf("round-trip metadata wrong: %+v", roundTrip) + } + if len(roundTrip.Results) != 2 { + t.Fatalf("round-trip results len = %d, want 2", len(roundTrip.Results)) + } + if roundTrip.Results[1].Status != "failed" || roundTrip.Results[1].Error != "ENOENT" { + t.Errorf("failed-result not preserved: %+v", roundTrip.Results[1]) + } +} + +// TestMigrationReport_PathCollisionResistance covers the sub-second +// collision-resistance edge case: two reports created in the same +// wall-clock second must produce distinct filenames. Without the +// random suffix (or with a non-crypto entropy source) a CI script +// that ran `nodeup upgrade` twice in a row would clobber the first +// report — defeating the "where did my migration go?" recovery flow. +func TestMigrationReport_PathCollisionResistance(t *testing.T) { + redirectDataDir(t) + + // Two reports created back-to-back. If both report the same + // timestamp (sub-second resolution), their filenames must still + // differ. + r1 := NewMigrationReport("fnm", "20.10.0", "20.11.0") + r2 := NewMigrationReport("fnm", "20.10.0", "20.11.0") + + p1, err := r1.Path() + if err != nil { + t.Fatalf("r1.Path: %v", err) + } + p2, err := r2.Path() + if err != nil { + t.Fatalf("r2.Path: %v", err) + } + if p1 == p2 { + t.Errorf("two reports within the same second produced the same path %q — collision risk", p1) + } + // Save both and verify both files exist independently on disk. + if err := r1.Save(); err != nil { + t.Fatalf("r1.Save: %v", err) + } + if err := r2.Save(); err != nil { + t.Fatalf("r2.Save: %v", err) + } + if _, err := os.Stat(p1); err != nil { + t.Errorf("r1 file missing after Save: %v", err) + } + if _, err := os.Stat(p2); err != nil { + t.Errorf("r2 file missing after Save: %v", err) + } }