address review comments

Signed-off-by: Stephanie <[email protected]>

pkg/devfile/generator/generators_test.go

@@ -201,26 +201,20 @@ func TestGetContainers(t *testing.T) {
 			// Unexpected error
 			if (err != nil) != tt.wantErr {
 				t.Errorf("TestGetContainers() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-
-			// Expected error and got an err
-			if tt.wantErr && err != nil {
-				return
-			}
-
-			for _, container := range containers {
-				if container.Name != tt.wantContainerName {
-					t.Errorf("TestGetContainers error: Name mismatch - got: %s, wanted: %s", container.Name, tt.wantContainerName)
-				}
-				if container.Image != tt.wantContainerImage {
-					t.Errorf("TestGetContainers error: Image mismatch - got: %s, wanted: %s", container.Image, tt.wantContainerImage)
-				}
-				if len(container.Env) > 0 && !reflect.DeepEqual(container.Env, tt.wantContainerEnv) {
-					t.Errorf("TestGetContainers error: Env mismatch - got: %+v, wanted: %+v", container.Env, tt.wantContainerEnv)
-				}
-				if len(container.VolumeMounts) > 0 && !reflect.DeepEqual(container.VolumeMounts, tt.wantContainerVolMount) {
-					t.Errorf("TestGetContainers error: Vol Mount mismatch - got: %+v, wanted: %+v", container.VolumeMounts, tt.wantContainerVolMount)
+			} else if err == nil {
+				for _, container := range containers {
+					if container.Name != tt.wantContainerName {
+						t.Errorf("TestGetContainers error: Name mismatch - got: %s, wanted: %s", container.Name, tt.wantContainerName)
+					}
+					if container.Image != tt.wantContainerImage {
+						t.Errorf("TestGetContainers error: Image mismatch - got: %s, wanted: %s", container.Image, tt.wantContainerImage)
+					}
+					if len(container.Env) > 0 && !reflect.DeepEqual(container.Env, tt.wantContainerEnv) {
+						t.Errorf("TestGetContainers error: Env mismatch - got: %+v, wanted: %+v", container.Env, tt.wantContainerEnv)
+					}
+					if len(container.VolumeMounts) > 0 && !reflect.DeepEqual(container.VolumeMounts, tt.wantContainerVolMount) {
+						t.Errorf("TestGetContainers error: Vol Mount mismatch - got: %+v, wanted: %+v", container.VolumeMounts, tt.wantContainerVolMount)
+					}
 				}
 			}
 		})

pkg/devfile/generator/utils_test.go

@@ -402,11 +402,11 @@ func TestAddSyncFolder(t *testing.T) {
 
 			if !tt.wantErr == (err != nil) {
 				t.Errorf("expected %v, actual %v", tt.wantErr, err)
-			}
-
-			for _, env := range container.Env {
-				if env.Name == EnvProjectsSrc && env.Value != tt.want {
-					t.Errorf("expected %s, actual %s", tt.want, env.Value)
+			} else if err == nil {
+				for _, env := range container.Env {
+					if env.Name == EnvProjectsSrc && env.Value != tt.want {
+						t.Errorf("expected %s, actual %s", tt.want, env.Value)
+					}
 				}
 			}
 		})
@@ -764,26 +764,20 @@ func TestGetServiceSpec(t *testing.T) {
 			// Unexpected error
 			if (err != nil) != tt.wantErr {
 				t.Errorf("TestGetServiceSpec() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-
-			// Expected error and got an err
-			if tt.wantErr && err != nil {
-				return
-			}
-
-			if !reflect.DeepEqual(serviceSpec.Selector, tt.labels) {
-				t.Errorf("expected service selector is %v, actual %v", tt.labels, serviceSpec.Selector)
-			}
-			if len(serviceSpec.Ports) != len(tt.wantPorts) {
-				t.Errorf("expected service ports length is %v, actual %v", len(tt.wantPorts), len(serviceSpec.Ports))
-			} else {
-				for i := range serviceSpec.Ports {
-					if serviceSpec.Ports[i].Name != tt.wantPorts[i].Name {
-						t.Errorf("expected name %s, actual name %s", tt.wantPorts[i].Name, serviceSpec.Ports[i].Name)
-					}
-					if serviceSpec.Ports[i].Port != tt.wantPorts[i].Port {
-						t.Errorf("expected port number is %v, actual %v", tt.wantPorts[i].Port, serviceSpec.Ports[i].Port)
+			} else if err == nil {
+				if !reflect.DeepEqual(serviceSpec.Selector, tt.labels) {
+					t.Errorf("expected service selector is %v, actual %v", tt.labels, serviceSpec.Selector)
+				}
+				if len(serviceSpec.Ports) != len(tt.wantPorts) {
+					t.Errorf("expected service ports length is %v, actual %v", len(tt.wantPorts), len(serviceSpec.Ports))
+				} else {
+					for i := range serviceSpec.Ports {
+						if serviceSpec.Ports[i].Name != tt.wantPorts[i].Name {
+							t.Errorf("expected name %s, actual name %s", tt.wantPorts[i].Name, serviceSpec.Ports[i].Name)
+						}
+						if serviceSpec.Ports[i].Port != tt.wantPorts[i].Port {
+							t.Errorf("expected port number is %v, actual %v", tt.wantPorts[i].Port, serviceSpec.Ports[i].Port)
+						}
 					}
 				}
 			}
@@ -1095,11 +1089,10 @@ func TestGetPortExposure(t *testing.T) {
 			}
 
 			mapCreated, err := getPortExposure(devObj, tt.filterOptions)
-			if !tt.wantErr && err != nil {
-				t.Errorf("TestGetPortExposure unexpected error: %v", err)
-			} else if tt.wantErr && err == nil {
-				t.Errorf("TestGetPortExposure expected error but got nil")
-			} else if !reflect.DeepEqual(mapCreated, tt.wantMap) {
+			// Checks for unexpected error cases
+			if !tt.wantErr == (err != nil) {
+				t.Errorf("TestGetPortExposure unexpected error %v, wantErr %v", err, tt.wantErr)
+			} else if err == nil && !reflect.DeepEqual(mapCreated, tt.wantMap) {
 				t.Errorf("TestGetPortExposure Expected: %v, got %v", tt.wantMap, mapCreated)
 			}
 
@@ -1233,16 +1226,16 @@ func TestGetPVCSpec(t *testing.T) {
 			// Checks for unexpected error cases
 			if !tt.wantErr == (err != nil) {
 				t.Errorf("resource.ParseQuantity unexpected error %v, wantErr %v", err, tt.wantErr)
-			}
-
-			pvcSpec := getPVCSpec(quantity)
-			if pvcSpec.AccessModes[0] != corev1.ReadWriteOnce {
-				t.Errorf("AccessMode Error: expected %s, actual %s", corev1.ReadWriteMany, pvcSpec.AccessModes[0])
-			}
+			} else if err == nil {
+				pvcSpec := getPVCSpec(quantity)
+				if pvcSpec.AccessModes[0] != corev1.ReadWriteOnce {
+					t.Errorf("AccessMode Error: expected %s, actual %s", corev1.ReadWriteMany, pvcSpec.AccessModes[0])
+				}
 
-			pvcSpecQuantity := pvcSpec.Resources.Requests["storage"]
-			if pvcSpecQuantity.String() != quantity.String() {
-				t.Errorf("pvcSpec.Resources.Requests Error: expected %v, actual %v", pvcSpecQuantity.String(), quantity.String())
+				pvcSpecQuantity := pvcSpec.Resources.Requests["storage"]
+				if pvcSpecQuantity.String() != quantity.String() {
+					t.Errorf("pvcSpec.Resources.Requests Error: expected %v, actual %v", pvcSpecQuantity.String(), quantity.String())
+				}
 			}
 		})
 	}

pkg/devfile/parser/data/v2/commands.go

@@ -66,19 +66,13 @@ func (d *DevfileV2) AddCommands(commands []v1.Command) error {
 // UpdateCommand updates the command with the given id
 // return an error if the command is not found
 func (d *DevfileV2) UpdateCommand(command v1.Command) error {
-	found := false
 	for i := range d.Commands {
 		if d.Commands[i].Id == command.Id {
-			found = true
 			d.Commands[i] = command
-			d.Commands[i].Id = d.Commands[i].Id
-			break
+			return nil
 		}
 	}
-	if !found {
-		return fmt.Errorf("update command failed: command %s not found", command.Id)
-	}
-	return nil
+	return fmt.Errorf("update command failed: command %s not found", command.Id)
 }
 
 // DeleteCommand removes the specified command

pkg/devfile/parser/data/v2/commands_test.go

@@ -295,11 +295,10 @@ func TestDevfile200_AddCommands(t *testing.T) {
 				},
 			}
 
-			got := d.AddCommands(tt.newCommands)
-			if !tt.wantErr && got != nil {
-				t.Errorf("TestDevfile200_AddCommands() unexpected error - %v", got)
-			} else if tt.wantErr && got == nil {
-				t.Errorf("TestDevfile200_AddCommands() wanted an error but got nil")
+			err := d.AddCommands(tt.newCommands)
+			// Unexpected error
+			if (err != nil) != tt.wantErr {
+				t.Errorf("TestDevfile200_AddCommands() error = %v, wantErr %v", err, tt.wantErr)
 			}
 		})
 	}
@@ -383,30 +382,26 @@ func TestDevfile200_UpdateCommands(t *testing.T) {
 			// Unexpected error
 			if (err != nil) != tt.wantErr {
 				t.Errorf("TestDevfile200_UpdateCommands() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if err != nil {
-				return
-			}
-
-			commands, err := d.GetCommands(common.DevfileOptions{})
-			if err != nil {
-				t.Errorf("TestDevfile200_UpdateCommands() unxpected error %v", err)
-				return
-			}
+			} else if err == nil {
+				commands, err := d.GetCommands(common.DevfileOptions{})
+				if err != nil {
+					t.Errorf("TestDevfile200_UpdateCommands() unxpected error %v", err)
+					return
+				}
 
-			matched := false
-			for _, devfileCommand := range commands {
-				if tt.newCommand.Id == devfileCommand.Id {
-					matched = true
-					if !reflect.DeepEqual(devfileCommand, tt.newCommand) {
-						t.Errorf("TestDevfile200_UpdateCommands() command mismatch - wanted %+v, got %+v", tt.newCommand, devfileCommand)
+				matched := false
+				for _, devfileCommand := range commands {
+					if tt.newCommand.Id == devfileCommand.Id {
+						matched = true
+						if !reflect.DeepEqual(devfileCommand, tt.newCommand) {
+							t.Errorf("TestDevfile200_UpdateCommands() command mismatch - wanted %+v, got %+v", tt.newCommand, devfileCommand)
+						}
 					}
 				}
-			}
 
-			if !matched {
-				t.Errorf("TestDevfile200_UpdateCommands() command mismatch - did not find command with id %s", tt.newCommand.Id)
+				if !matched {
+					t.Errorf("TestDevfile200_UpdateCommands() command mismatch - did not find command with id %s", tt.newCommand.Id)
+				}
 			}
 		})
 	}
@@ -483,7 +478,6 @@ func TestDeleteCommands(t *testing.T) {
 			err := d.DeleteCommand(tt.commandToDelete)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("DeleteCommand() error = %v, wantErr %v", err, tt.wantErr)
-				return
 			} else if err == nil {
 				assert.Equal(t, tt.wantCommands, d.Commands, "The two values should be the same.")
 			}

pkg/devfile/parser/data/v2/common/command_helper_test.go

@@ -324,9 +324,7 @@ func TestGetCommandType(t *testing.T) {
 			// Unexpected error
 			if (err != nil) != tt.wantErr {
 				t.Errorf("TestGetCommandType() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if got != tt.commandType {
+			} else if err == nil && got != tt.commandType {
 				t.Errorf("TestGetCommandType error: command type mismatch, expected: %v got: %v", tt.commandType, got)
 			}
 		})

pkg/devfile/parser/data/v2/common/component_helper_test.go

@@ -179,9 +179,7 @@ func TestGetComponentType(t *testing.T) {
 			// Unexpected error
 			if (err != nil) != tt.wantErr {
 				t.Errorf("TestGetComponentType() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if got != tt.componentType {
+			} else if err == nil && got != tt.componentType {
 				t.Errorf("TestGetComponentType error: component type mismatch, expected: %v got: %v", tt.componentType, got)
 			}
 		})

pkg/devfile/parser/data/v2/common/options_test.go

@@ -63,11 +63,10 @@ func TestFilterDevfileObject(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			filterIn, err := FilterDevfileObject(tt.attributes, tt.options)
-			if !tt.wantErr && err != nil {
-				t.Errorf("TestFilterDevfileObject unexpected error - %v", err)
-			} else if tt.wantErr && err == nil {
-				t.Errorf("TestFilterDevfileObject wanted error got nil")
-			} else if filterIn != tt.wantFilter {
+			// Unexpected error
+			if (err != nil) != tt.wantErr {
+				t.Errorf("TestFilterDevfileObject() error = %v, wantErr %v", err, tt.wantErr)
+			} else if err == nil && filterIn != tt.wantFilter {
 				t.Errorf("TestFilterDevfileObject error - expected %v got %v", tt.wantFilter, filterIn)
 			}
 		})

pkg/devfile/parser/data/v2/common/project_helper_test.go

@@ -103,16 +103,16 @@ func TestGitLikeProjectSource_GetDefaultSource(t *testing.T) {
 			got1, got2, got3, err := GetDefaultSource(tt.gitLikeProjectSource)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("GitLikeProjectSource.GetDefaultSource() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if got1 != tt.want1 {
-				t.Errorf("GitLikeProjectSource.GetDefaultSource() got1 = %v, want %v", got1, tt.want1)
-			}
-			if got2 != tt.want2 {
-				t.Errorf("GitLikeProjectSource.GetDefaultSource() got2 = %v, want %v", got2, tt.want2)
-			}
-			if got3 != tt.want3 {
-				t.Errorf("GitLikeProjectSource.GetDefaultSource() got2 = %v, want %v", got3, tt.want3)
+			} else if err == nil {
+				if got1 != tt.want1 {
+					t.Errorf("GitLikeProjectSource.GetDefaultSource() got1 = %v, want %v", got1, tt.want1)
+				}
+				if got2 != tt.want2 {
+					t.Errorf("GitLikeProjectSource.GetDefaultSource() got2 = %v, want %v", got2, tt.want2)
+				}
+				if got3 != tt.want3 {
+					t.Errorf("GitLikeProjectSource.GetDefaultSource() got2 = %v, want %v", got3, tt.want3)
+				}
 			}
 		})
 	}
@@ -170,9 +170,7 @@ func TestGetProjectSrcType(t *testing.T) {
 			// Unexpected error
 			if (err != nil) != tt.wantErr {
 				t.Errorf("TestGetProjectSrcType() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if got != tt.projectSrcType {
+			} else if err == nil && got != tt.projectSrcType {
 				t.Errorf("TestGetProjectSrcType error: project src type mismatch, expected: %v got: %v", tt.projectSrcType, got)
 			}
 		})

pkg/devfile/parser/data/v2/components.go

@@ -90,18 +90,13 @@ func (d *DevfileV2) AddComponents(components []v1.Component) error {
 // UpdateComponent updates the component with the given name
 // return an error if the component is not found
 func (d *DevfileV2) UpdateComponent(component v1.Component) error {
-	found := false
 	for i := range d.Components {
 		if d.Components[i].Name == component.Name {
 			d.Components[i] = component
-			found = true
-			break
+			return nil
 		}
 	}
-	if !found {
-		return fmt.Errorf("update component failed: component %s not found", component.Name)
-	}
-	return nil
+	return fmt.Errorf("update component failed: component %s not found", component.Name)
 }
 
 // DeleteComponent removes the specified component