[update] Add simple unit test for lockfile merge logic (#1309)

## Summary

I'm sorry if this steps a bit on your toes :( . I just figured I wanted
some unit test before I made the changes to the merging logic, and
noticed that if I just refactored a function a little bit, then we could
add some unit tests easily at the function level.

I just added 1 unit test here to demonstrate, and will add more later as
I implement the bugfix.

## How was it tested?
Unit tests

Author: ipince

internal/impl/devbox_test.go

@@ -67,15 +67,7 @@ func (n *testNix) PrintDevEnv(ctx context.Context, args *nix.PrintDevEnvArgs) (*
 }
 
 func TestComputeNixEnv(t *testing.T) {
-	path := t.TempDir()
-	_, err := devconfig.Init(path, os.Stdout)
-	require.NoError(t, err, "InitConfig should not fail")
-	d, err := Open(&devopt.Opts{
-		Dir:    path,
-		Writer: os.Stdout,
-		Pure:   false,
-	})
-	require.NoError(t, err, "Open should not fail")
+	d := devboxForTesting(t)
 	d.nix = &testNix{}
 	ctx := context.Background()
 	env, err := d.computeNixEnv(ctx, false /*use cache*/)
@@ -84,15 +76,7 @@ func TestComputeNixEnv(t *testing.T) {
 }
 
 func TestComputeNixPathIsIdempotent(t *testing.T) {
-	dir := t.TempDir()
-	_, err := devconfig.Init(dir, os.Stdout)
-	require.NoError(t, err, "InitConfig should not fail")
-	devbox, err := Open(&devopt.Opts{
-		Dir:    dir,
-		Writer: os.Stdout,
-		Pure:   false,
-	})
-	require.NoError(t, err, "Open should not fail")
+	devbox := devboxForTesting(t)
 	devbox.nix = &testNix{"/tmp/my/path"}
 	ctx := context.Background()
 	env, err := devbox.computeNixEnv(ctx, false /*use cache*/)
@@ -114,15 +98,7 @@ func TestComputeNixPathIsIdempotent(t *testing.T) {
 }
 
 func TestComputeNixPathWhenRemoving(t *testing.T) {
-	dir := t.TempDir()
-	_, err := devconfig.Init(dir, os.Stdout)
-	require.NoError(t, err, "InitConfig should not fail")
-	devbox, err := Open(&devopt.Opts{
-		Dir:    dir,
-		Writer: os.Stdout,
-		Pure:   false,
-	})
-	require.NoError(t, err, "Open should not fail")
+	devbox := devboxForTesting(t)
 	devbox.nix = &testNix{"/tmp/my/path"}
 	ctx := context.Background()
 	env, err := devbox.computeNixEnv(ctx, false /*use cache*/)
@@ -145,3 +121,17 @@ func TestComputeNixPathWhenRemoving(t *testing.T) {
 
 	assert.NotEqual(t, path, path2, "path should not be the same")
 }
+
+func devboxForTesting(t *testing.T) *Devbox {
+	path := t.TempDir()
+	_, err := devconfig.Init(path, os.Stdout)
+	require.NoError(t, err, "InitConfig should not fail")
+	d, err := Open(&devopt.Opts{
+		Dir:    path,
+		Writer: os.Stdout,
+		Pure:   false,
+	})
+	require.NoError(t, err, "Open should not fail")
+
+	return d
+}

internal/impl/update.go

@@ -89,25 +89,35 @@ func (d *Devbox) updateDevboxPackage(
 	ctx context.Context,
 	pkg *devpkg.Package,
 ) error {
-	existing := d.lockfile.Packages[pkg.Raw]
-	newEntry, err := d.lockfile.FetchResolvedPackage(pkg.Raw)
+	resolved, err := d.lockfile.FetchResolvedPackage(pkg.Raw)
 	if err != nil {
 		return err
 	}
+
+	return d.mergeResolvedPackageToLockfile(ctx, pkg, resolved, d.lockfile)
+}
+
+func (d *Devbox) mergeResolvedPackageToLockfile(
+	ctx context.Context,
+	pkg *devpkg.Package,
+	resolved *lock.Package,
+	lockfile *lock.File,
+) error {
+	existing := lockfile.Packages[pkg.Raw]
 	if existing == nil {
-		ux.Finfo(d.writer, "Resolved %s to %[1]s %[2]s\n", pkg, newEntry.Resolved)
-		d.lockfile.Packages[pkg.Raw] = newEntry
+		ux.Finfo(d.writer, "Resolved %s to %[1]s %[2]s\n", pkg, resolved.Resolved)
+		lockfile.Packages[pkg.Raw] = resolved
 		return nil
 	}
 
-	if existing.Version != newEntry.Version {
-		ux.Finfo(d.writer, "Updating %s %s -> %s\n", pkg, existing.Version, newEntry.Version)
+	if existing.Version != resolved.Version {
+		ux.Finfo(d.writer, "Updating %s %s -> %s\n", pkg, existing.Version, resolved.Version)
 		if err := d.removePackagesFromProfile(ctx, []string{pkg.Raw}); err != nil {
 			// Warn but continue. TODO(landau): ensurePackagesAreInstalled should
 			// sync the profile so we don't need to do this manually.
 			ux.Fwarning(d.writer, "Failed to remove %s from profile: %s\n", pkg, err)
 		}
-		d.lockfile.Packages[pkg.Raw] = newEntry
+		lockfile.Packages[pkg.Raw] = resolved
 		return nil
 	}
 
@@ -118,15 +128,15 @@ func (d *Devbox) updateDevboxPackage(
 			return err
 		}
 
-		// If the newEntry has a system info for the user's system, then add/overwrite it. We don't overwrite
+		// If the resolved has a system info for the user's system, then add/overwrite it. We don't overwrite
 		// other system infos because we don't want to clobber system-dependent CAStorePaths.
-		if newSysInfo, ok := newEntry.Systems[userSystem]; ok {
+		if newSysInfo, ok := resolved.Systems[userSystem]; ok {
 			if !newSysInfo.Equals(existing.Systems[userSystem]) {
 				// We only guard this so that the ux messaging is accurate. We could overwrite every time.
-				if d.lockfile.Packages[pkg.Raw].Systems == nil {
-					d.lockfile.Packages[pkg.Raw].Systems = map[string]*lock.SystemInfo{}
+				if lockfile.Packages[pkg.Raw].Systems == nil {
+					lockfile.Packages[pkg.Raw].Systems = map[string]*lock.SystemInfo{}
 				}
-				d.lockfile.Packages[pkg.Raw].Systems[userSystem] = newSysInfo
+				lockfile.Packages[pkg.Raw].Systems[userSystem] = newSysInfo
 				ux.Finfo(d.writer, "Updated system information for %s\n", pkg)
 				return nil
 			}

internal/impl/update_test.go

@@ -0,0 +1,30 @@
+package impl
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"go.jetpack.io/devbox/internal/devpkg"
+	"go.jetpack.io/devbox/internal/lock"
+)
+
+func TestUpdateNewPackageIsAdded(t *testing.T) {
+	devbox := devboxForTesting(t)
+
+	raw := "[email protected]"
+	devPkg := &devpkg.Package{
+		Raw: raw,
+	}
+	resolved := &lock.Package{
+		Resolved: "resolved-flake-reference",
+	}
+	lockfile := &lock.File{
+		Packages: map[string]*lock.Package{}, // empty
+	}
+
+	err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile)
+	require.NoError(t, err, "update failed")
+
+	require.Contains(t, lockfile.Packages, raw)
+}