Improve unit tests for internal/summon/command

internal/plugin/resolver.go

@@ -124,7 +124,7 @@ func (resolver *Resolver) resolveForProvider(
 	return provider, providerResponses, ""
 }
 
-// Resolve accepts an list of Providers and a list of Credentials and
+// Resolve accepts n list of Providers and a list of Credentials and
 // attempts to obtain the value of each Credential from the appropriate Provider.
 func (resolver *Resolver) Resolve(credentials []*config_v2.Credential) (map[string][]byte, error) {
 	if len(credentials) == 0 {
@@ -174,7 +174,7 @@ func (resolver *Resolver) Resolve(credentials []*config_v2.Credential) (map[stri
 		// Sort the error strings to provide deterministic output
 		sort.Strings(errorStrings)
 
-		err = fmt.Errorf(strings.Join(errorStrings, "; "))
+		err = fmt.Errorf(strings.Join(errorStrings, "\n"))
 	}
 
 	return result, err

internal/plugin/resolver_test.go

@@ -63,7 +63,7 @@ func Test_Resolver(t *testing.T) {
 			So(err.Error(), ShouldEqual,
 				"ERROR: Resolving credentials from provider 'env' failed: "+
 					"env cannot find environment variable 'something-also-not-in-env', "+
-					"env cannot find environment variable 'something-not-in-env'; "+
+					"env cannot find environment variable 'something-not-in-env'\n"+
 					"ERROR: Resolving credentials from provider 'file' failed: "+
 					"open something-not-on-file: no such file or directory")
 		})

internal/plugin/v1/provider_mock.go

@@ -0,0 +1,57 @@
+package v1
+
+import (
+	"errors"
+	"strings"
+)
+
+// MockProvider conforms to, and allows testing of, both the singleValueProvider and
+// Provider interfaces
+type MockProvider struct {
+	GetValueCallArgs []string // keeps track of args for each call to getValue
+}
+
+// GetValue returns
+// 0. If [id] has prefix 'err_', returns (nil, errors.New(id + "_value"))
+// 1. Otherwise, returns ([]byte(id + "_value"), nil)
+func (p *MockProvider) GetValue(id string) ([]byte, error) {
+	p.GetValueCallArgs = append(p.GetValueCallArgs, id)
+
+	if strings.HasPrefix(id, "err_") {
+		return nil, errors.New(id + "_value")
+	}
+	return []byte(id + "_value"), nil
+}
+
+// GetValues sequentially get values for unique ids by calling GetValue
+//
+// If there exists any id with the prefix 'global_err_', the function will return
+// (nil, errors.New(id + "_value"))
+func (p *MockProvider) GetValues(ids ...string) (
+	map[string]ProviderResponse,
+	error,
+) {
+	responses := map[string]ProviderResponse{}
+
+	for _, id := range ids {
+		if _, ok := responses[id]; ok {
+			continue
+		}
+
+		if strings.HasPrefix(id, "global_err_") {
+			return nil, errors.New(id + "_value")
+		}
+
+		pr := ProviderResponse{}
+		pr.Value, pr.Error = p.GetValue(id)
+
+		responses[id] = pr
+	}
+
+	return responses, nil
+}
+
+// GetName simply returns "mock-provider"
+func (p *MockProvider) GetName() string {
+	return "mock-provider"
+}

internal/plugin/v1/provider_mock_test.go

@@ -0,0 +1,48 @@
+package v1
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestMockProvider_GetName(t *testing.T) {
+	p := &MockProvider{}
+
+	assert.Equal(t, p.GetName(), "mock-provider")
+}
+
+func TestMockProvider_GetValue(t *testing.T) {
+	p := &MockProvider{}
+
+	t.Run("Get value", func(t *testing.T) {
+		val, err := p.GetValue("foo")
+		if !assert.NoError(t, err) {
+			return
+		}
+
+		assert.Equal(t, string(val), "foo_value")
+	})
+
+	t.Run("Reports error", func(t *testing.T) {
+		val, err := p.GetValue("err_foo")
+		assert.EqualError(t, err, "err_foo_value")
+		assert.Nil(t, val)
+	})
+}
+
+func TestMockProvider_GetValues(t *testing.T) {
+	p := &MockProvider{}
+
+	t.Run("Sequentially calls GetValue", func(t *testing.T) {
+		_, _ = p.GetValues("a", "b", "c")
+
+		assert.Equal(t, []string{"a", "b", "c"}, p.GetValueCallArgs)
+	})
+
+	t.Run("Returns global error", func(t *testing.T) {
+		res, err := p.GetValues("a", "b", "global_err_example", "c")
+		assert.EqualError(t, err, "global_err_example_value")
+		assert.Nil(t, res)
+	})
+}

internal/plugin/v1/provider_test.go

@@ -1,49 +1,29 @@
 package v1
 
 import (
-	"errors"
-	"strings"
 	"testing"
 
 	. "github.com/smartystreets/goconvey/convey"
 )
 
-// mockProvider conforms to the singleValueProvider interface and allows testing the
-// singleValueProvider interface
-type mockProvider struct {
-	getValueCallArgs []string // keeps track of args for each call to getValue
-}
-
-// GetValue returns
-// 0. If [id] has prefix 'err_', returns (nil, errors.New(id))
-// 1. Otherwise, returns ([]byte(id), nil)
-func (p *mockProvider) GetValue(id string) ([]byte, error) {
-	p.getValueCallArgs = append(p.getValueCallArgs, id)
-
-	if strings.HasPrefix(id, "err_") {
-		return nil, errors.New(id)
-	}
-	return []byte(id), nil
-}
-
 func TestGetValues(t *testing.T) {
 	Convey("GetValues", t, func() {
 		// ids, 4 of which are unique
 		ids := []string{"foo", "err_meow", "bar", "bar", "err_meow", "err_baz"}
 
 		Convey("Sequentially call GetValue on unique ids", func() {
-			p := &mockProvider{}
+			p := &MockProvider{}
 			_, _ = GetValues(
 				p,
 				ids...,
 			)
 
-			So(p.getValueCallArgs, ShouldResemble, []string{"foo", "err_meow", "bar", "err_baz"})
+			So(p.GetValueCallArgs, ShouldResemble, []string{"foo", "err_meow", "bar", "err_baz"})
 		})
 
 		Convey("Returns good or bad responses depending on unique ids", func() {
 			providerResponses, err := GetValues(
-				&mockProvider{},
+				&MockProvider{},
 				ids...,
 			)
 			So(err, ShouldBeNil)
@@ -59,18 +39,18 @@ func TestGetValues(t *testing.T) {
 
 // ensureGoodResponse ensures [key] exists within a provider-responses map, and that the
 // entry has no error and the value is []byte(key), in line with how
-// mockProvider#GetValue works
+// MockProvider#GetValue works
 func ensureGoodResponse(responses map[string]ProviderResponse, key string) {
 	So(responses, ShouldContainKey, key)
 	So(responses[key].Error, ShouldBeNil)
-	So(responses[key].Value, ShouldResemble, []byte(key))
+	So(responses[key].Value, ShouldResemble, []byte(key+"_value"))
 }
 
 // ensureErrResponse ensures [key] exists within a provider-responses map, and that the
 // entry has no value and the error string is equal to [key], in line with how
-// mockProvider#GetValue works
+// MockProvider#GetValue works
 func ensureErrResponse(responses map[string]ProviderResponse, key string) {
 	So(responses, ShouldContainKey, key)
 	So(responses[key].Value, ShouldBeNil)
-	So(responses[key].Error.Error(), ShouldEqual, key)
+	So(responses[key].Error.Error(), ShouldEqual, key+"_value")
 }

internal/summon/command/run.go

@@ -5,6 +5,7 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"sort"
 	"strings"
 
 	"github.com/cyberark/summon/secretsyml"
@@ -26,13 +27,28 @@ type Subcommand struct {
 
 // buildEnvironment builds the environment strings from the map of secrets values, along with the
 // secrets configuration metadata and the temp files location.
-func buildEnvironment(secrets map[string]string, secretsMap secretsyml.SecretsMap, tempFactory *TempFactory) (env []string, err error) {
-	env = make([]string, len(secrets))
-	for key, value := range secrets {
-		envvar := formatForEnv(key, value, secretsMap[key], tempFactory)
+func buildEnvironment(
+	secrets map[string]string,
+	secretsMap secretsyml.SecretsMap,
+	tempFactory *TempFactory,
+) ([]string, error) {
+	env := make([]string, 0, len(secrets))
+	keys := make([]string, 0, len(secrets))
+
+	for k := range secrets {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+
+	for _, key := range keys {
+		envvar, err := formatForEnv(key, secrets[key], secretsMap[key], tempFactory)
+		if err != nil {
+			return nil, err
+		}
 		env = append(env, envvar)
 	}
-	return
+
+	return env, nil
 }
 
 // resolveSecrets obtains the value of each requested secret.
@@ -54,7 +70,8 @@ func resolveSecrets(provider plugin_v1.Provider, secretsMap secretsyml.SecretsMa
 		}
 	}
 
-	if atLeastOneVar := len(varSecretsSpecPaths) > 0; !atLeastOneVar {
+	// If there are no variables to resolve, return what we have
+	if len(varSecretsSpecPaths) == 0 {
 		return result, nil
 	}
 
@@ -73,6 +90,9 @@ func resolveSecrets(provider plugin_v1.Provider, secretsMap secretsyml.SecretsMa
 		}
 	}
 	if len(errorStrings) > 0 {
+		// Sort the error strings to provide deterministic output
+		sort.Strings(errorStrings)
+
 		err = fmt.Errorf(strings.Join(errorStrings, "\n"))
 		return nil, err
 	}
@@ -111,13 +131,22 @@ func (sc *Subcommand) runSubcommand(env []string) (err error) {
 
 // formatForEnv returns a string in %k=%v format, where %k=namespace of the secret and
 // %v=the secret value or path to a temporary file containing the secret
-func formatForEnv(key string, value string, spec secretsyml.SecretSpec, tempFactory *TempFactory) string {
+func formatForEnv(
+	key string,
+	value string,
+	spec secretsyml.SecretSpec,
+	tempFactory *TempFactory,
+) (string, error) {
 	if spec.IsFile() {
-		fname := tempFactory.Push(value)
+		fname, err := tempFactory.Push(value)
+		if err != nil {
+			return "", err
+		}
+
 		value = fname
 	}
 
-	return fmt.Sprintf("%s=%s", key, value)
+	return fmt.Sprintf("%s=%s", key, value), nil
 }
 
 // Run encapsulates the logic of Action without cli Context for easier testing