fix: update ValidateBricks to return all validation errors as a slice and adjust related tests

Author: dido18

internal/orchestrator/app/parser.go

@@ -141,35 +141,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, allowing the caller to see all issues at once rather than stopping at the first error.
+// errors found as a slice, 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 = errors.Join(allErrors, fmt.Errorf("brick %q not found", appBrick.ID))
+			allErrors = append(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 = errors.Join(allErrors, fmt.Errorf("variable %q does not exist on brick %q", appBrickName, indexBrick.ID))
+				allErrors = append(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 = errors.Join(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID))
+					allErrors = append(allErrors, fmt.Errorf("variable %q is required by brick %q", indexBrickVariable.Name, indexBrick.ID))
 				}
 			}
 		}

internal/orchestrator/app/validator_test.go

@@ -1,11 +1,11 @@
 package app
 
 import (
+	"errors"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"go.bug.st/f"
 
 	"github.com/arduino/go-paths-helper"
 
@@ -18,49 +18,58 @@ func TestValidateAppDescriptorBricks(t *testing.T) {
 	require.NotNil(t, bricksIndex)
 
 	testCases := []struct {
-		name        string
-		filename    string
-		expectedErr *string
+		name           string
+		filename       string
+		expectedErrors []error // Now expecting a slice of error messages
 	}{
 		{
-			name:        "valid with all required filled",
-			filename:    "all-required-app.yaml",
-			expectedErr: nil,
+			name:           "valid with all required filled",
+			filename:       "all-required-app.yaml",
+			expectedErrors: nil,
 		},
 		{
-			name:        "valid with missing bricks",
-			filename:    "no-bricks-app.yaml",
-			expectedErr: nil,
+			name:           "valid with missing bricks",
+			filename:       "no-bricks-app.yaml",
+			expectedErrors: nil,
 		},
 		{
-			name:        "valid with empty list of bricks",
-			filename:    "empty-bricks-app.yaml",
-			expectedErr: nil,
+			name:           "valid with empty list of bricks",
+			filename:       "empty-bricks-app.yaml",
+			expectedErrors: nil,
 		},
 		{
-			name:        "valid if required variable is empty string",
-			filename:    "empty-required-app.yaml",
-			expectedErr: nil,
+			name:           "valid if required variable is empty string",
+			filename:       "empty-required-app.yaml",
+			expectedErrors: nil,
 		},
 		{
-			name:        "invalid if required variable is omitted",
-			filename:    "omitted-required-app.yaml",
-			expectedErr: f.Ptr("variable \"ARDUINO_DEVICE_ID\" is required by brick \"arduino:arduino_cloud\"\nvariable \"ARDUINO_SECRET\" is required by brick \"arduino:arduino_cloud\""),
+			name:     "invalid if required variable is omitted",
+			filename: "omitted-required-app.yaml",
+			expectedErrors: []error{
+				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",
-			expectedErr: f.Ptr("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 brick id not found",
-			filename:    "not-found-brick-app.yaml",
-			expectedErr: f.Ptr("brick \"arduino:not_existing_brick\" not found"),
+			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 variable does not exist in the brick",
-			filename:    "not-found-variable-app.yaml",
-			expectedErr: f.Ptr("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",
+			expectedErrors: []error{
+				errors.New("variable \"NOT_EXISTING_VARIABLE\" does not exist on brick \"arduino:arduino_cloud\""),
+			},
 		},
 	}
 
@@ -69,12 +78,13 @@ func TestValidateAppDescriptorBricks(t *testing.T) {
 			appDescriptor, err := ParseDescriptorFile(paths.New("testdata/validator/" + tc.filename))
 			require.NoError(t, err)
 
-			err = appDescriptor.ValidateBricks(bricksIndex)
-			if tc.expectedErr == nil {
-				assert.NoError(t, err)
+			errs := appDescriptor.ValidateBricks(bricksIndex)
+			if tc.expectedErrors == nil {
+				require.Nil(t, errs)
+				assert.Empty(t, errs, "Expected no validation errors")
 			} else {
-				require.Error(t, err)
-				assert.Equal(t, *tc.expectedErr, err.Error())
+				require.Len(t, errs, len(tc.expectedErrors), "Expected %d errors but got %d", len(tc.expectedErrors), len(errs))
+				require.Exactly(t, tc.expectedErrors, errs)
 			}
 		})
 	}