From 9f4b87c616a0329cb6c8ff27db003766ef0bba2c Mon Sep 17 00:00:00 2001 From: Sam Berning <113054166+sam-berning@users.noreply.github.com> Date: Tue, 13 Jun 2023 10:50:41 -0700 Subject: [PATCH] test: mocks LimaUser to fix race condition in support bundle unit tests (#450) Issue #, if available: https://github.com/runfinch/finch/issues/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 --- cmd/finch/main.go | 8 ++-- pkg/lima/wrapper/lima_wrapper.go | 30 +++++++++++++++ pkg/mocks/lima_wrapper.go | 50 +++++++++++++++++++++++++ pkg/support/support.go | 7 +++- pkg/support/support_test.go | 64 ++++++++++++++++++++++++++++---- 5 files changed, 146 insertions(+), 13 deletions(-) create mode 100644 pkg/lima/wrapper/lima_wrapper.go create mode 100644 pkg/mocks/lima_wrapper.go diff --git a/cmd/finch/main.go b/cmd/finch/main.go index 799c0ec96..66125ca01 100644 --- a/cmd/finch/main.go +++ b/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 diff --git a/pkg/lima/wrapper/lima_wrapper.go b/pkg/lima/wrapper/lima_wrapper.go new file mode 100644 index 000000000..48a25f50d --- /dev/null +++ b/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) +} diff --git a/pkg/mocks/lima_wrapper.go b/pkg/mocks/lima_wrapper.go new file mode 100644 index 000000000..a56856f17 --- /dev/null +++ b/pkg/mocks/lima_wrapper.go @@ -0,0 +1,50 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/runfinch/finch/pkg/lima/wrapper (interfaces: LimaWrapper) + +// Package mocks is a generated GoMock package. +package mocks + +import ( + user "os/user" + reflect "reflect" + + gomock "github.com/golang/mock/gomock" +) + +// MockLimaWrapper is a mock of LimaWrapper interface. +type MockLimaWrapper struct { + ctrl *gomock.Controller + recorder *MockLimaWrapperMockRecorder +} + +// MockLimaWrapperMockRecorder is the mock recorder for MockLimaWrapper. +type MockLimaWrapperMockRecorder struct { + mock *MockLimaWrapper +} + +// NewMockLimaWrapper creates a new mock instance. +func NewMockLimaWrapper(ctrl *gomock.Controller) *MockLimaWrapper { + mock := &MockLimaWrapper{ctrl: ctrl} + mock.recorder = &MockLimaWrapperMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockLimaWrapper) EXPECT() *MockLimaWrapperMockRecorder { + return m.recorder +} + +// LimaUser mocks base method. +func (m *MockLimaWrapper) LimaUser(arg0 bool) (*user.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "LimaUser", arg0) + ret0, _ := ret[0].(*user.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// LimaUser indicates an expected call of LimaUser. +func (mr *MockLimaWrapperMockRecorder) LimaUser(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "LimaUser", reflect.TypeOf((*MockLimaWrapper)(nil).LimaUser), arg0) +} diff --git a/pkg/support/support.go b/pkg/support/support.go index bd5149269..4d5b54022 100644 --- a/pkg/support/support.go +++ b/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,6 +61,7 @@ func NewBundleBuilder( config BundleConfig, finch fpath.Finch, ecc command.Creator, + lima wrapper.LimaWrapper, ) BundleBuilder { return &bundleBuilder{ logger: logger, @@ -67,6 +69,7 @@ func NewBundleBuilder( 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 } diff --git a/pkg/support/support_test.go b/pkg/support/support_test.go index f512fcdb6..33c16c30e 100644 --- a/pkg/support/support_test.go +++ b/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)