chore: changes from review

Incorporated changes from code review. Signed-off-by: Richard Case <[email protected]>
liquidmetal-dev · Jul 28, 2021 · ca1731e · ca1731e
1 parent cbdd874
commit ca1731e
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 61 deletions.
diff --git a/pkg/provider/registry/errors.go b/pkg/provider/registry/errors.go
diff --git a/pkg/provider/registry/registry.go b/pkg/provider/registry/registry.go
@@ -18,8 +18,8 @@ func RegisterProvider(name string, factory provider.Factory) error {
 	pluginsLock.Lock()
 	defer pluginsLock.Unlock()
 
-	if _, exists := plugins[name]; exists {
-		return ErrDuplicatePlugin
+	if _, ok := plugins[name]; ok {
+		return fmt.Errorf("plugin %s has already been registered", name) //nolint:goerr113
 	}
 
 	plugins[name] = factory
@@ -33,23 +33,21 @@ func GetPluginInstance(ctx context.Context, name string, runtime *provider.Runti
 	pluginsLock.RLock()
 	defer pluginsLock.RUnlock()
 
-	if factoryFunc, exists := plugins[name]; exists {
+	if factoryFunc, ok := plugins[name]; ok {
 		return factoryFunc(ctx, runtime)
 	}
 
-	return nil, fmt.Errorf("getting plugin %s: %w", name, ErrPluginNotFound)
+	return nil, fmt.Errorf("plugin %s not found", name) // nolint:goerr113
 }
 
 // ListPlugins returns a list of the plugin names that have been registered.
 func ListPlugins() []string {
 	pluginsLock.RLock()
 	defer pluginsLock.RUnlock()
 
-	names := make([]string, len(plugins))
-	i := 0
+	names := make([]string, 0, len(plugins))
 	for name := range plugins {
-		names[i] = name
-		i++
+		names = append(names, name)
 	}
 
 	return names

diff --git a/pkg/provider/registry/registry_test.go b/pkg/provider/registry/registry_test.go
@@ -14,35 +14,53 @@ import (
 )
 
 func TestRegistry_RegisterProvider(t *testing.T) {
-	RegisterTestingT(t)
-	t.Cleanup(registry.Reset)
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
+	testCases := []struct {
+		name              string
+		pluginsToRegister []string
+		expectRegError    []bool
+	}{
+		{
+			name:              "No plugins registered",
+			pluginsToRegister: []string{"prov1"},
+			expectRegError:    []bool{false},
+		},
+		{
+			name:              "Plugin already registered",
+			pluginsToRegister: []string{"prov1", "prov1"},
+			expectRegError:    []bool{false, true},
+		},
+	}
 
-	err := registry.RegisterProvider("prov1", mockRegisterPluginFactor(false, ctrl))
-	Expect(err).NotTo(HaveOccurred())
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			RegisterTestingT(t)
+			ctrl := gomock.NewController(t)
+			defer ctrl.Finish()
+			defer registry.Reset()
+
+			Expect(len(tc.pluginsToRegister)).To(Equal(len(tc.expectRegError)), "length of pluginsToRegister must be the same as expectRegError")
+
+			for i := range tc.pluginsToRegister {
+				pluginName := tc.pluginsToRegister[i]
+				expectErr := tc.expectRegError[i]
+
+				actualErr := registry.RegisterProvider(pluginName, mockRegisterPluginFactor(false, ctrl))
+				if expectErr {
+					Expect(actualErr).To(HaveOccurred())
+				} else {
+					Expect(actualErr).ToNot(HaveOccurred())
+				}
+			}
+		})
+	}
 }
 
-func TestRegistry_RegisterProviderDuplicate(t *testing.T) {
+func TestRegistry_GetInstance(t *testing.T) {
 	RegisterTestingT(t)
-	t.Cleanup(registry.Reset)
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
 
-	providerName := "prov1"
-
-	err := registry.RegisterProvider(providerName, mockRegisterPluginFactor(false, ctrl))
-	Expect(err).NotTo(HaveOccurred())
-
-	err = registry.RegisterProvider(providerName, mockRegisterPluginFactor(false, ctrl))
-	Expect(err).To(HaveOccurred())
-}
-
-func TestRegistry_GetInstance(t *testing.T) {
-	RegisterTestingT(t)
 	t.Cleanup(registry.Reset)
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
+	t.Cleanup(ctrl.Finish)
 
 	providerName := "prov1"
 	factoryCalled := false
@@ -61,9 +79,10 @@ func TestRegistry_GetInstance(t *testing.T) {
 
 func TestRegistry_GetInstanceNotExist(t *testing.T) {
 	RegisterTestingT(t)
-	t.Cleanup(registry.Reset)
 	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
+
+	t.Cleanup(registry.Reset)
+	t.Cleanup(ctrl.Finish)
 
 	providerName := "prov1"
 
@@ -72,28 +91,40 @@ func TestRegistry_GetInstanceNotExist(t *testing.T) {
 	Expect(p).To(BeNil())
 }
 
-func TestRegistry_ListAllEmpty(t *testing.T) {
+func TestRegistry_ListAll(t *testing.T) {
 	RegisterTestingT(t)
-	t.Cleanup(registry.Reset)
-
-	registered := registry.ListPlugins()
-	Expect(registered).To(HaveLen(0))
-}
 
-func TestRegistry_ListAllNotEmpty(t *testing.T) {
-	RegisterTestingT(t)
-	t.Cleanup(registry.Reset)
-	ctrl := gomock.NewController(t)
-	defer ctrl.Finish()
+	testCases := []struct {
+		name              string
+		registeredPlugins []string
+		expectedLen       int
+	}{
+		{
+			name:        "No plugins registered",
+			expectedLen: 0,
+		},
+		{
+			name:              "Plugins registered",
+			registeredPlugins: []string{"prov1", "prov2"},
+			expectedLen:       2,
+		},
+	}
 
-	err := registry.RegisterProvider("prov1", mockRegisterPluginFactor(false, ctrl))
-	Expect(err).NotTo(HaveOccurred())
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+			defer ctrl.Finish()
+			defer registry.Reset()
 
-	err = registry.RegisterProvider("prov2", mockRegisterPluginFactor(false, ctrl))
-	Expect(err).NotTo(HaveOccurred())
+			for _, pluginName := range tc.registeredPlugins {
+				err := registry.RegisterProvider(pluginName, mockRegisterPluginFactor(false, ctrl))
+				Expect(err).NotTo(HaveOccurred())
+			}
 
-	registered := registry.ListPlugins()
-	Expect(registered).To(HaveLen(2))
+			registered := registry.ListPlugins()
+			Expect(registered).To(HaveLen(tc.expectedLen))
+		})
+	}
 }
 
 func mockRegisterPluginFactor(shouldFailCreate bool, ctrl *gomock.Controller) provider.Factory {