test: mocks LimaUser to fix race condition in support bundle unit tes…

…ts (#450)

Issue #, if available: #447

*Description of changes:*

There was a race condition in the unit tests for `support-bundle
generate` caused by Lima's `osutil.LimaUser` not being thread-safe. I
don't think there's really a need to make it thread-safe, so I think
it's easier to just wrap and mock it for the unit tests, which I've done
here.

*Testing done:*

```
make test-unit
```


- [x] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

Signed-off-by: Sam Berning <bernings@amazon.com>

cmd/finch/main.go

@@ -9,6 +9,9 @@ import (
 	"io"
 	"os"
 
+	"github.com/spf13/afero"
+	"github.com/spf13/cobra"
+
 	"github.com/runfinch/finch/pkg/command"
 	"github.com/runfinch/finch/pkg/config"
 	"github.com/runfinch/finch/pkg/dependency"
@@ -17,13 +20,11 @@ import (
 	"github.com/runfinch/finch/pkg/flog"
 	"github.com/runfinch/finch/pkg/fmemory"
 	"github.com/runfinch/finch/pkg/fssh"
+	"github.com/runfinch/finch/pkg/lima/wrapper"
 	"github.com/runfinch/finch/pkg/path"
 	"github.com/runfinch/finch/pkg/support"
 	"github.com/runfinch/finch/pkg/system"
 	"github.com/runfinch/finch/pkg/version"
-
-	"github.com/spf13/afero"
-	"github.com/spf13/cobra"
 )
 
 const finchRootCmd = "finch"
@@ -94,6 +95,7 @@ var newApp = func(logger flog.Logger, fp path.Finch, fs afero.Fs, fc *config.Fin
 		support.NewBundleConfig(fp, system.NewStdLib().Env("HOME")),
 		fp,
 		ecc,
+		wrapper.NewLimaWrapper(),
 	)
 
 	// append nerdctl commands

pkg/lima/wrapper/lima_wrapper.go

@@ -0,0 +1,30 @@
+// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+// SPDX-License-Identifier: Apache-2.0
+
+// Package wrapper provides an interface to wrap Lima utils
+package wrapper
+
+import (
+	"os/user"
+
+	"github.com/lima-vm/lima/pkg/osutil"
+)
+
+// LimaWrapper provides Lima utils in an interface to facilitate mocking
+//
+//go:generate mockgen --destination=../../mocks/lima_wrapper.go -package=mocks github.com/runfinch/finch/pkg/lima/wrapper LimaWrapper
+type LimaWrapper interface {
+	LimaUser(warn bool) (*user.User, error)
+}
+
+type limaWrapper struct{}
+
+// NewLimaWrapper returns a new LimaWrapper.
+func NewLimaWrapper() LimaWrapper {
+	return &limaWrapper{}
+}
+
+// LimaUser returns the user that will be used inside the Lima VM.
+func (*limaWrapper) LimaUser(warn bool) (*user.User, error) {
+	return osutil.LimaUser(warn)
+}

pkg/support/support.go

@@ -15,12 +15,12 @@ import (
 	"strings"
 	"time"
 
-	"github.com/lima-vm/lima/pkg/osutil"
 	"github.com/spf13/afero"
 	"gopkg.in/yaml.v3"
 
 	"github.com/runfinch/finch/pkg/command"
 	"github.com/runfinch/finch/pkg/flog"
+	"github.com/runfinch/finch/pkg/lima/wrapper"
 	fpath "github.com/runfinch/finch/pkg/path"
 	"github.com/runfinch/finch/pkg/version"
 )
@@ -51,6 +51,7 @@ type bundleBuilder struct {
 	config BundleConfig
 	finch  fpath.Finch
 	ecc    command.Creator
+	lima   wrapper.LimaWrapper
 }
 
 // NewBundleBuilder produces a new BundleBuilder.
@@ -60,13 +61,15 @@ func NewBundleBuilder(
 	config BundleConfig,
 	finch fpath.Finch,
 	ecc command.Creator,
+	lima wrapper.LimaWrapper,
 ) BundleBuilder {
 	return &bundleBuilder{
 		logger: logger,
 		fs:     fs,
 		config: config,
 		finch:  finch,
 		ecc:    ecc,
+		lima:   lima,
 	}
 }
 
@@ -171,7 +174,7 @@ func (bb *bundleBuilder) copyInFile(writer *zip.Writer, fileName string, prefix
 			return err
 		}
 
-		user, err := osutil.LimaUser(false)
+		user, err := bb.lima.LimaUser(false)
 		if err != nil {
 			return err
 		}

pkg/support/support_test.go

@@ -5,6 +5,7 @@ package support
 
 import (
 	"archive/zip"
+	"os/user"
 	"testing"
 	"time"
 
@@ -25,23 +26,34 @@ func TestSupport_NewBundleBuilder(t *testing.T) {
 	logger := mocks.NewLogger(ctrl)
 	fs := afero.NewMemMapFs()
 	finch := fpath.Finch("mockfinch")
+	lima := mocks.NewMockLimaWrapper(ctrl)
 
 	config := NewBundleConfig(finch, "mockhome")
-	NewBundleBuilder(logger, fs, config, finch, ecc)
+	NewBundleBuilder(logger, fs, config, finch, ecc, lima)
 }
 
 func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 	t.Parallel()
 
+	mockUser := &user.User{
+		Username: "mockuser",
+	}
+
 	testCases := []struct {
 		name    string
-		mockSvc func(*mocks.Logger, *mocks.BundleConfig, *mocks.CommandCreator, *mocks.Command)
+		mockSvc func(*mocks.Logger, *mocks.BundleConfig, *mocks.CommandCreator, *mocks.Command, *mocks.MockLimaWrapper)
 		include []string
 		exclude []string
 	}{
 		{
 			name: "Generate support bundle",
-			mockSvc: func(logger *mocks.Logger, config *mocks.BundleConfig, ecc *mocks.CommandCreator, cmd *mocks.Command) {
+			mockSvc: func(
+				logger *mocks.Logger,
+				config *mocks.BundleConfig,
+				ecc *mocks.CommandCreator,
+				cmd *mocks.Command,
+				lima *mocks.MockLimaWrapper,
+			) {
 				logger.EXPECT().Debugf("Creating %s...", gomock.Any())
 				logger.EXPECT().Debugln("Gathering platform data...")
 
@@ -67,13 +79,21 @@ func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 				logger.EXPECT().Debugf("Copying %s...", "config1")
 				logger.EXPECT().Debugf("Copying %s...", "config2")
 				logger.EXPECT().Debugln("Copying in additional files...")
+
+				lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
 			},
 			include: []string{},
 			exclude: []string{},
 		},
 		{
 			name: "Generate support bundle with an extra file included",
-			mockSvc: func(logger *mocks.Logger, config *mocks.BundleConfig, ecc *mocks.CommandCreator, cmd *mocks.Command) {
+			mockSvc: func(
+				logger *mocks.Logger,
+				config *mocks.BundleConfig,
+				ecc *mocks.CommandCreator,
+				cmd *mocks.Command,
+				lima *mocks.MockLimaWrapper,
+			) {
 				logger.EXPECT().Debugf("Creating %s...", gomock.Any())
 				logger.EXPECT().Debugln("Gathering platform data...")
 
@@ -96,13 +116,21 @@ func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 				logger.EXPECT().Debugf("Copying %s...", "config1")
 				logger.EXPECT().Debugln("Copying in additional files...")
 				logger.EXPECT().Debugf("Copying %s...", "extra1")
+
+				lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
 			},
 			include: []string{"extra1"},
 			exclude: []string{},
 		},
 		{
 			name: "Generate support bundle with a log file excluded",
-			mockSvc: func(logger *mocks.Logger, config *mocks.BundleConfig, ecc *mocks.CommandCreator, cmd *mocks.Command) {
+			mockSvc: func(
+				logger *mocks.Logger,
+				config *mocks.BundleConfig,
+				ecc *mocks.CommandCreator,
+				cmd *mocks.Command,
+				lima *mocks.MockLimaWrapper,
+			) {
 				logger.EXPECT().Debugf("Creating %s...", gomock.Any())
 				logger.EXPECT().Debugln("Gathering platform data...")
 
@@ -124,13 +152,21 @@ func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 				logger.EXPECT().Debugln("Copying in config files...")
 				logger.EXPECT().Debugf("Copying %s...", "config1")
 				logger.EXPECT().Debugln("Copying in additional files...")
+
+				lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
 			},
 			include: []string{},
 			exclude: []string{"log1"},
 		},
 		{
 			name: "Generate support bundle with a config file excluded",
-			mockSvc: func(logger *mocks.Logger, config *mocks.BundleConfig, ecc *mocks.CommandCreator, cmd *mocks.Command) {
+			mockSvc: func(
+				logger *mocks.Logger,
+				config *mocks.BundleConfig,
+				ecc *mocks.CommandCreator,
+				cmd *mocks.Command,
+				lima *mocks.MockLimaWrapper,
+			) {
 				logger.EXPECT().Debugf("Creating %s...", gomock.Any())
 				logger.EXPECT().Debugln("Gathering platform data...")
 
@@ -152,13 +188,21 @@ func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 				logger.EXPECT().Debugln("Copying in config files...")
 				logger.EXPECT().Infof("Excluding %s...", "config1")
 				logger.EXPECT().Debugln("Copying in additional files...")
+
+				lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
 			},
 			include: []string{},
 			exclude: []string{"config1"},
 		},
 		{
 			name: "Generate support bundle with an included file excluded",
-			mockSvc: func(logger *mocks.Logger, config *mocks.BundleConfig, ecc *mocks.CommandCreator, cmd *mocks.Command) {
+			mockSvc: func(
+				logger *mocks.Logger,
+				config *mocks.BundleConfig,
+				ecc *mocks.CommandCreator,
+				cmd *mocks.Command,
+				lima *mocks.MockLimaWrapper,
+			) {
 				logger.EXPECT().Debugf("Creating %s...", gomock.Any())
 				logger.EXPECT().Debugln("Gathering platform data...")
 
@@ -181,6 +225,8 @@ func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 				logger.EXPECT().Debugf("Copying %s...", "config1")
 				logger.EXPECT().Debugln("Copying in additional files...")
 				logger.EXPECT().Infof("Excluding %s...", "extra1")
+
+				lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
 			},
 			include: []string{"extra1"},
 			exclude: []string{"extra1"},
@@ -198,6 +244,7 @@ func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 			config := mocks.NewBundleConfig(ctrl)
 			finch := fpath.Finch("mockfinch")
 			ecc := mocks.NewCommandCreator(ctrl)
+			lima := mocks.NewMockLimaWrapper(ctrl)
 			cmd := mocks.NewCommand(ctrl)
 
 			builder := &bundleBuilder{
@@ -206,6 +253,7 @@ func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 				config: config,
 				finch:  finch,
 				ecc:    ecc,
+				lima:   lima,
 			}
 
 			testFiles := []string{
@@ -225,7 +273,7 @@ func TestSupportBundleBuilder_GenerateSupportBundle(t *testing.T) {
 				require.NoError(t, err)
 			}
 
-			tc.mockSvc(logger, config, ecc, cmd)
+			tc.mockSvc(logger, config, ecc, cmd, lima)
 
 			zipFile, err := builder.GenerateSupportBundle(tc.include, tc.exclude)
 			assert.NoError(t, err)