refactor: separate out task group constructor

internal/task/dependency_graph_builder_test.go

@@ -0,0 +1,89 @@
+package task
+
+import (
+	"testing"
+
+	"github.com/leg100/pug/internal/resource"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type fakeTaskCreator struct{}
+
+func (f *fakeTaskCreator) Create(spec Spec) (*Task, error) {
+	return (&factory{}).newTask(spec)
+}
+
+func TestNewGroupWithDependencies(t *testing.T) {
+	vpcID := resource.NewID(resource.Module)
+	mysqlID := resource.NewID(resource.Module)
+	redisID := resource.NewID(resource.Module)
+	backendID := resource.NewID(resource.Module)
+	frontendID := resource.NewID(resource.Module)
+	mqID := resource.NewID(resource.Module)
+
+	vpcSpec := Spec{ModuleID: &vpcID, Dependencies: &Dependencies{}}
+	mysqlSpec := Spec{ModuleID: &mysqlID, Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID}}}
+	redisSpec := Spec{ModuleID: &redisID, Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID}}}
+	backendSpec := Spec{ModuleID: &backendID, Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID, mysqlID, redisID}}}
+	frontendSpec := Spec{ModuleID: &frontendID, Dependencies: &Dependencies{ModuleIDs: []resource.ID{vpcID, backendID}}}
+	mqSpec := Spec{ModuleID: &mqID, Dependencies: &Dependencies{}}
+
+	t.Run("normal order", func(t *testing.T) {
+		got, err := createDependentTasks(&fakeTaskCreator{}, false,
+			vpcSpec,
+			mysqlSpec,
+			redisSpec,
+			backendSpec,
+			frontendSpec,
+			mqSpec,
+		)
+		require.NoError(t, err)
+
+		if assert.Len(t, got, 6) {
+			vpcTask := hasDependencies(t, got, vpcID) // 0 dependencies
+			mysqlTask := hasDependencies(t, got, mysqlID, vpcTask)
+			redisTask := hasDependencies(t, got, redisID, vpcTask)
+			backendTask := hasDependencies(t, got, backendID, vpcTask, mysqlTask, redisTask)
+			_ = hasDependencies(t, got, frontendID, vpcTask, backendTask)
+			_ = hasDependencies(t, got, mqID)
+		}
+	})
+
+	t.Run("reverse order", func(t *testing.T) {
+		got, err := createDependentTasks(&fakeTaskCreator{}, true,
+			vpcSpec,
+			mysqlSpec,
+			redisSpec,
+			backendSpec,
+			frontendSpec,
+			mqSpec,
+		)
+		require.NoError(t, err)
+
+		if assert.Len(t, got, 6) {
+			frontendTask := hasDependencies(t, got, frontendID) // 0 dependencies
+			backendTask := hasDependencies(t, got, backendID, frontendTask)
+			mysqlTask := hasDependencies(t, got, mysqlID, backendTask)
+			redisTask := hasDependencies(t, got, redisID, backendTask)
+			_ = hasDependencies(t, got, vpcID, mysqlTask, redisTask, backendTask, frontendTask)
+			_ = hasDependencies(t, got, mqID)
+		}
+	})
+}
+
+func hasDependencies(t *testing.T, got []*Task, want resource.ID, deps ...resource.ID) resource.ID {
+	for _, task := range got {
+		if task.ModuleID != nil && *task.ModuleID == want {
+			// Module matches, so now check dependencies
+			if assert.Len(t, task.DependsOn, len(deps)) {
+				for _, dep := range deps {
+					assert.Contains(t, task.DependsOn, dep)
+				}
+				return task.ID
+			}
+		}
+	}
+	t.Fatalf("%s not found in %v", want, got)
+	return resource.ID{}
+}

internal/task/group.go

@@ -1,6 +1,8 @@
 package task
 
 import (
+	"errors"
+	"fmt"
 	"slices"
 	"time"
 
@@ -16,6 +18,72 @@ type Group struct {
 	CreateErrors []error
 }
 
+func newGroup(service *Service, specs ...Spec) (*Group, error) {
+	if len(specs) == 0 {
+		return nil, errors.New("no specs provided")
+	}
+	g := &Group{
+		ID:      resource.NewID(resource.TaskGroup),
+		Created: time.Now(),
+	}
+	// Validate specifications. There are some settings that are incompatible
+	// with one another within a task group.
+	var (
+		respectModuleDependencies *bool
+		inverseDependencyOrder    *bool
+	)
+	for _, spec := range specs {
+		// All specs must specify Dependencies or not specify Dependencies.
+		deps := (spec.Dependencies != nil)
+		if respectModuleDependencies == nil {
+			respectModuleDependencies = &deps
+		} else if *respectModuleDependencies != deps {
+			return nil, fmt.Errorf("not all specs share same respect-module-dependencies setting")
+		}
+		// All specs specifying dependencies must set InverseDependencyOrder to
+		// the same value
+		inverse := (spec.Dependencies != nil && spec.Dependencies.InverseDependencyOrder)
+		if inverseDependencyOrder == nil {
+			inverseDependencyOrder = &inverse
+		} else if *inverseDependencyOrder != inverse {
+			return nil, fmt.Errorf("not all specs share same inverse-dependency-order setting")
+		}
+	}
+	if *respectModuleDependencies {
+		tasks, err := createDependentTasks(service, *inverseDependencyOrder, specs...)
+		if err != nil {
+			return nil, err
+		}
+		g.Tasks = tasks
+	} else {
+		for _, spec := range specs {
+			task, err := service.Create(spec)
+			if err != nil {
+				g.CreateErrors = append(g.CreateErrors, err)
+				continue
+			}
+			g.Tasks = append(g.Tasks, task)
+		}
+	}
+	if len(g.Tasks) == 0 {
+		return g, errors.New("all tasks failed to be created")
+	}
+
+	for _, task := range g.Tasks {
+		if g.Command == "" {
+			g.Command = task.String()
+		} else if g.Command != task.String() {
+			// Detected that not all tasks have the same command, so name the
+			// task group to reflect that multiple commands comprise the group.
+			//
+			// TODO: make a constant
+			g.Command = "multi"
+		}
+	}
+
+	return g, nil
+}
+
 func (g *Group) String() string { return g.Command }
 
 func (g *Group) IncludesTask(taskID resource.ID) bool {

internal/task/service.go

@@ -1,10 +1,7 @@
 package task
 
 import (
-	"errors"
-	"fmt"
 	"slices"
-	"time"
 
 	"github.com/leg100/pug/internal"
 	"github.com/leg100/pug/internal/logging"
@@ -97,66 +94,9 @@ func (s *Service) Create(spec Spec) (*Task, error) {
 // Create a task group from one or more task specs. An error is returned if zero
 // specs are provided, or if it fails to create at least one task.
 func (s *Service) CreateGroup(specs ...Spec) (*Group, error) {
-	if len(specs) == 0 {
-		return nil, errors.New("no specs provided")
-	}
-	g := &Group{
-		ID:      resource.NewID(resource.TaskGroup),
-		Created: time.Now(),
-	}
-	// Validate specifications. There are some settings that are incompatible
-	// with one another within a task group.
-	var (
-		respectModuleDependencies *bool
-		inverseDependencyOrder    *bool
-	)
-	for _, spec := range specs {
-		// All specs must specify Dependencies or not specify Dependencies.
-		deps := (spec.Dependencies != nil)
-		if respectModuleDependencies == nil {
-			respectModuleDependencies = &deps
-		} else if *respectModuleDependencies != deps {
-			return nil, fmt.Errorf("not all specs share same respect-module-dependencies setting")
-		}
-		// All specs specifying dependencies must set InverseDependencyOrder to
-		// the same value
-		inverse := (spec.Dependencies != nil && spec.Dependencies.InverseDependencyOrder)
-		if inverseDependencyOrder == nil {
-			inverseDependencyOrder = &inverse
-		} else if *inverseDependencyOrder != inverse {
-			return nil, fmt.Errorf("not all specs share same inverse-dependency-order setting")
-		}
-	}
-	if *respectModuleDependencies {
-		tasks, err := createDependentTasks(s, *inverseDependencyOrder, specs...)
-		if err != nil {
-			return nil, err
-		}
-		g.Tasks = tasks
-	} else {
-		for _, spec := range specs {
-			task, err := s.Create(spec)
-			if err != nil {
-				g.CreateErrors = append(g.CreateErrors, err)
-				continue
-			}
-			g.Tasks = append(g.Tasks, task)
-		}
-	}
-	if len(g.Tasks) == 0 {
-		return g, errors.New("all tasks failed to be created")
-	}
-
-	for _, task := range g.Tasks {
-		if g.Command == "" {
-			g.Command = task.String()
-		} else if g.Command != task.String() {
-			// Detected that not all tasks have the same command, so name the
-			// task group to reflect that multiple commands comprise the group.
-			//
-			// TODO: make a constant
-			g.Command = "multi"
-		}
+	g, err := newGroup(s, specs...)
+	if err != nil {
+		return nil, err
 	}
 
 	s.logger.Debug("created task group", "group", g)