fix: properly handle errors in upgrade auto-approve path

Author: kkemple

cmd/upgrade/upgrade_test.go

@@ -72,3 +72,38 @@ func TestUpgradeCommand(t *testing.T) {
 	}
 	updatePkgMock.AssertCalled(t, "CheckForUpdates", mock.Anything, mock.Anything, true)
 }
+
+func TestUpgradeCommandWithAutoApproveError(t *testing.T) {
+	// Create a mock of UpdateNotification that returns an error on InstallUpdatesWithoutPrompt
+	originalCheckForUpdates := checkForUpdatesFunc
+	defer func() {
+		checkForUpdatesFunc = originalCheckForUpdates
+	}()
+
+	// Create mocks
+	ctx := slackcontext.MockContext(t.Context())
+	clientsMock := shared.NewClientsMock()
+
+	// Create clients that is mocked for testing
+	clients := shared.NewClientFactory(clientsMock.MockClientFactory())
+
+	// Mock the checkForUpdates function to simulate an error during auto-approve updates
+	checkForUpdatesFunc = func(clients *shared.ClientFactory, cmd *cobra.Command, autoApprove bool) error {
+		if autoApprove {
+			return assert.AnError // Simulate error when auto-approve is true
+		}
+		return nil
+	}
+
+	// Create the command with auto-approve flag
+	cmd := NewCommand(clients)
+	testutil.MockCmdIO(clients.IO, cmd)
+	cmd.SetArgs([]string{"--auto-approve"})
+
+	// Execute the command and verify it returns the error
+	err := cmd.ExecuteContext(ctx)
+
+	// Verify the error was properly propagated
+	assert.Error(t, err)
+	assert.Equal(t, assert.AnError, err)
+}

internal/update/update_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/slackapi/slack-cli/internal/shared"
 	"github.com/slackapi/slack-cli/internal/slackdeps"
 	"github.com/spf13/cobra"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
@@ -109,3 +110,107 @@ func Test_Update_HasUpdate(t *testing.T) {
 		})
 	}
 }
+
+func Test_Update_InstallUpdatesWithoutPrompt(t *testing.T) {
+	for name, tt := range map[string]struct {
+		dependencyHasUpdate   []bool
+		installUpdateErrors   []error
+		expectedErrorContains string
+	}{
+		"No dependencies have updates": {
+			dependencyHasUpdate:   []bool{false, false},
+			installUpdateErrors:   []error{nil, nil},
+			expectedErrorContains: "",
+		},
+		"Dependencies have updates, all install successfully": {
+			dependencyHasUpdate:   []bool{true, true},
+			installUpdateErrors:   []error{nil, nil},
+			expectedErrorContains: "",
+		},
+		"Dependencies have updates, first fails to install": {
+			dependencyHasUpdate:   []bool{true, false},
+			installUpdateErrors:   []error{assert.AnError, nil},
+			expectedErrorContains: "general error for testing",
+		},
+		"Dependencies have updates, second fails to install": {
+			dependencyHasUpdate:   []bool{true, true},
+			installUpdateErrors:   []error{nil, assert.AnError},
+			expectedErrorContains: "general error for testing",
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			// Create clients
+			clients := shared.ClientFactory{
+				Config:    config.NewConfig(slackdeps.NewFsMock(), slackdeps.NewOsMock()),
+				SDKConfig: hooks.NewSDKConfigMock(),
+			}
+
+			// Create the mocks
+			var dependencies []Dependency
+
+			// Special handling for "first fails to install" case
+			if name == "Dependencies have updates, first fails to install" {
+				// Only create and add the first dependency since execution will stop after it fails
+				mockDep1 := new(mockDependency)
+				mockDep1.On("HasUpdate").Return(true, nil)
+				mockDep1.On("PrintUpdateNotification", mock.Anything).Return(false, nil)
+				mockDep1.On("InstallUpdate", mock.Anything).Return(assert.AnError)
+				dependencies = append(dependencies, mockDep1)
+
+				// Second dependency is never called due to early return
+				mockDep2 := new(mockDependency)
+				dependencies = append(dependencies, mockDep2)
+			} else {
+				// Handle all other test cases normally
+				for i, hasUpdate := range tt.dependencyHasUpdate {
+					mockDep := new(mockDependency)
+					mockDep.On("HasUpdate").Return(hasUpdate, nil)
+
+					// Only set expectations for PrintUpdateNotification and InstallUpdate
+					// if the dependency hasUpdate = true
+					if hasUpdate {
+						mockDep.On("PrintUpdateNotification", mock.Anything).Return(false, nil)
+						mockDep.On("InstallUpdate", mock.Anything).Return(tt.installUpdateErrors[i])
+					}
+
+					dependencies = append(dependencies, mockDep)
+				}
+			}
+
+			// Create updateNotification
+			updateNotification = &UpdateNotification{
+				clients:      &clients,
+				enabled:      true,
+				envDisabled:  "SLACK_SKIP_UPDATE",
+				hoursToWait:  defaultHoursToWait,
+				dependencies: dependencies,
+			}
+
+			// Create test cmd
+			cmd := &cobra.Command{}
+
+			// Test
+			err := updateNotification.InstallUpdatesWithoutPrompt(cmd)
+
+			if tt.expectedErrorContains != "" {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tt.expectedErrorContains)
+			} else {
+				require.NoError(t, err)
+			}
+
+			// Only verify expectations on the first dependency for the "first fails" case
+			// since the second dependency should never be called
+			if name == "Dependencies have updates, first fails to install" {
+				mockDep1 := dependencies[0].(*mockDependency)
+				mockDep1.AssertExpectations(t)
+			} else {
+				// Verify all expectations were met for other cases
+				for _, dep := range dependencies {
+					mockDep := dep.(*mockDependency)
+					mockDep.AssertExpectations(t)
+				}
+			}
+		})
+	}
+}