fix: refactor ValidateBricks to return a single error and update related tests for consistency

Author: dido18

internal/orchestrator/app/parser.go

@@ -142,35 +142,35 @@ func (a *AppDescriptor) IsValid() error {
 
 // ValidateBricks checks that all bricks referenced in the given AppDescriptor exist in the provided BricksIndex,
 // and that all required variables for each brick are present and valid. It collects and returns all validation
-// errors found as a slice, allowing the caller to see all issues at once rather than stopping at the first error.
+// errors as a single joined error, allowing the caller to see all issues at once rather than stopping at the first error.
 // If the index is nil, validation is skipped and nil is returned.
-func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) []error {
+func (a *AppDescriptor) ValidateBricks(index *bricksindex.BricksIndex) error {
 	if index == nil {
 		return nil
 	}
 
-	var allErrors []error
+	var allErrors error
 
 	for _, appBrick := range a.Bricks {
 		indexBrick, found := index.FindBrickByID(appBrick.ID)
 		if !found {
-			allErrors = append(allErrors, fmt.Errorf("brick %q not found", appBrick.ID))
+			allErrors = errors.Join(allErrors, fmt.Errorf("brick %q not found", appBrick.ID))
 			continue // Skip further validation for this brick since it doesn't exist
 		}
 
 		// Check that all app variables exist in brick definition
 		for appBrickName := range appBrick.Variables {
 			_, exist := indexBrick.GetVariable(appBrickName)
 			if !exist {
-				allErrors = append(allErrors, fmt.Errorf("variable %q does not exist on brick %q", appBrickName, indexBrick.ID))
+				allErrors = errors.Join(allErrors, fmt.Errorf("variable %q does not exist on brick %q", appBrickName, indexBrick.ID))
 			}
 		}
 
 		// Check that all required brick variables are provided by app
 		for _, indexBrickVariable := range indexBrick.Variables {
 			if indexBrickVariable.IsRequired() {
 				if _, exist := appBrick.Variables[indexBrickVariable.Name]; !exist {
-					allErrors = append(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID))
+					allErrors = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID))
 				}
 			}
 		}

internal/orchestrator/app/validator_test.go

@@ -18,58 +18,52 @@ func TestValidateAppDescriptorBricks(t *testing.T) {
 	require.NotNil(t, bricksIndex)
 
 	testCases := []struct {
-		name           string
-		filename       string
-		expectedErrors []error // Now expecting a slice of error messages
+		name          string
+		filename      string
+		expectedError error
 	}{
 		{
-			name:           "valid with all required filled",
-			filename:       "all-required-app.yaml",
-			expectedErrors: nil,
+			name:          "valid with all required filled",
+			filename:      "all-required-app.yaml",
+			expectedError: nil,
 		},
 		{
-			name:           "valid with missing bricks",
-			filename:       "no-bricks-app.yaml",
-			expectedErrors: nil,
+			name:          "valid with missing bricks",
+			filename:      "no-bricks-app.yaml",
+			expectedError: nil,
 		},
 		{
-			name:           "valid with empty list of bricks",
-			filename:       "empty-bricks-app.yaml",
-			expectedErrors: nil,
+			name:          "valid with empty list of bricks",
+			filename:      "empty-bricks-app.yaml",
+			expectedError: nil,
 		},
 		{
-			name:           "valid if required variable is empty string",
-			filename:       "empty-required-app.yaml",
-			expectedErrors: nil,
+			name:          "valid if required variable is empty string",
+			filename:      "empty-required-app.yaml",
+			expectedError: nil,
 		},
 		{
 			name:     "invalid if required variable is omitted",
 			filename: "omitted-required-app.yaml",
-			expectedErrors: []error{
+			expectedError: errors.Join(
 				errors.New("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\""),
 				errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
-			},
+			),
 		},
 		{
-			name:     "invalid if a required variable among two is omitted",
-			filename: "omitted-mixed-required-app.yaml",
-			expectedErrors: []error{
-				errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
-			},
+			name:          "invalid if a required variable among two is omitted",
+			filename:      "omitted-mixed-required-app.yaml",
+			expectedError: errors.New("variable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
 		},
 		{
-			name:     "invalid if brick id not found",
-			filename: "not-found-brick-app.yaml",
-			expectedErrors: []error{
-				errors.New("brick \"arduino:not_existing_brick\" not found"),
-			},
+			name:          "invalid if brick id not found",
+			filename:      "not-found-brick-app.yaml",
+			expectedError: errors.New("brick \"arduino:not_existing_brick\" not found"),
 		},
 		{
-			name:     "invalid if variable does not exist in the brick",
-			filename: "not-found-variable-app.yaml",
-			expectedErrors: []error{
-				errors.New("variable \"NOT_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\""),
-			},
+			name:          "invalid if variable does not exist in the brick",
+			filename:      "not-found-variable-app.yaml",
+			expectedError: errors.New("variable \"NOT_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\""),
 		},
 	}
 
@@ -78,13 +72,12 @@ func TestValidateAppDescriptorBricks(t *testing.T) {
 			appDescriptor, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename))
 			require.NoError(t, err)
 
-			errs := appDescriptor.ValidateBricks(bricksIndex)
-			if tc.expectedErrors == nil {
-				require.Nil(t, errs)
-				assert.Empty(t, errs, "Expected no validation errors")
+			err = appDescriptor.ValidateBricks(bricksIndex)
+			if tc.expectedError == nil {
+				assert.NoError(t, err, "Expected no validation errors")
 			} else {
-				require.Len(t, errs, len(tc.expectedErrors), "Expected %d errors but got %d", len(tc.expectedErrors), len(errs))
-				require.Exactly(t, tc.expectedErrors, errs)
+				require.Error(t, err, "Expected validation error")
+				assert.Equal(t, tc.expectedError.Error(), err.Error(), "Error message should match")
 			}
 		})
 	}

internal/orchestrator/orchestrator.go

@@ -122,13 +122,9 @@ func StartApp(
 		ctx, cancel := context.WithCancel(ctx)
 		defer cancel()
 
-		errs := app.Descriptor.ValidateBricks(bricksIndex)
-		if errs != nil {
-			for _, e := range errs {
-				if !yield(StreamMessage{error: e}) {
-					return
-				}
-			}
+		err := app.Descriptor.ValidateBricks(bricksIndex)
+		if err != nil {
+			yield(StreamMessage{error: err})
 			return
 		}
 
@@ -457,13 +453,9 @@ func RestartApp(
 		ctx, cancel := context.WithCancel(ctx)
 		defer cancel()
 
-		errs := appToStart.Descriptor.ValidateBricks(bricksIndex)
-		if errs != nil {
-			for _, e := range errs {
-				if !yield(StreamMessage{error: e}) {
-					return
-				}
-			}
+		err := appToStart.Descriptor.ValidateBricks(bricksIndex)
+		if err != nil {
+			yield(StreamMessage{error: err})
 			return
 		}