Fix reorder of nodes with multiple dependsOn nodes

Makefile

@@ -64,6 +64,10 @@ format:
 check:
 	@$(REPO_ROOT)/vendor/github.com/gardener/gardener/hack/check.sh ./cmd/... ./pkg/...
 
+.PHONY: test
+test:
+	@go test -mod=vendor $(REPO_ROOT)/cmd/... $(REPO_ROOT)/pkg/...
+
 .PHONY: install
 install:
 	@./hack/install

pkg/testmachinery/testdefinition/types.go

@@ -13,7 +13,6 @@ import (
 // TestDefinition represents a TestDefinition which was fetched from locations.
 type TestDefinition struct {
 	Info     *tmv1beta1.TestDefinition
-	TaskName string
 	Location Location
 	FileName string
 	Template *argov1.Template

pkg/testmachinery/testflow/flow_operations.go

@@ -74,10 +74,9 @@ func ReorderChildrenOfNodes(list *node.Set) *node.Set {
 // reorderSerialNodes reorders all children of a node so that serial steps run in serial after parallel nodes.
 // The functions returns the new Children
 func reorderChildrenOfNode(root *node.Node) *node.Set {
-	grandChildren := root.Children.GetChildren()
 
-	// directly return if there is only one node in the pool
-	if root.Children.Len() == 1 {
+	// directly return if there are no nodes or only one node in the pool
+	if root.Children.Len() <= 1 {
 		// todo: write test for special case
 		return root.Children
 	}
@@ -101,33 +100,48 @@ func reorderChildrenOfNode(root *node.Node) *node.Set {
 	root.ClearChildren()
 	root.AddChildren(parallelNodes.List()...)
 
+	previousChildren := parallelNodes.GetChildren()
+	// remove children from parallel node and the root note
+	parallelNodes.Remove(previousChildren.List()...)
+	root.RemoveChildren(previousChildren.List()...)
 	for i, serialNode := range serialNodes.List() {
+		// remove the current node from the list of previous children
+		// and add all children of the current node
+		previousChildren.Remove(serialNode)
+		previousChildren.Add(serialNode.Children.List()...)
+		// remove the serial as possible parent from all previous children
+		previousChildren.RemoveParents(serialNode)
+		serialNode.ClearChildren()
 		if i == 0 {
 			parallelNodes.ClearChildren()
 			parallelNodes.AddChildren(serialNode)
 
-			serialNode.ClearParents()
+			// only remove the root parent as otherwise other parent information will get lost
+			serialNode.RemoveParent(root)
 			serialNode.AddParents(parallelNodes.List()...)
 		} else {
 			prevNode := serialNodes.List()[i-1]
 
-			prevNode.ClearChildren()
+			//prevNode.ClearChildren()
 			prevNode.AddChildren(serialNode)
 
-			serialNode.ClearParents()
+			// only remove the root parent as otherwise other parent information will get lost
+			serialNode.RemoveParent(root)
 			serialNode.AddParents(prevNode)
 		}
 
+		// last node
 		if i == serialNodes.Len()-1 {
 			serialNode.ClearChildren()
-			serialNode.AddChildren(grandChildren.List()...)
+			serialNode.AddChildren(previousChildren.List()...)
 
-			grandChildren.ClearParents()
-			grandChildren.AddParents(serialNode)
+			previousChildren.AddParents(serialNode)
 		}
 	}
 
-	return grandChildren
+	// return last node
+	// the list cannot be empty as this case is already checked above.
+	return node.NewSet(serialNodes.List()[serialNodes.Len()-1])
 }
 
 // ApplyOutputScope defines the artifact scopes for outputs.
@@ -282,6 +296,9 @@ func getJointNodes(nodes []*node.Node, branches []*node.Set, getNext func(*node.
 // Note the order of the node sets are essential.
 func findJointNode(nodeSets []*node.Set) *node.Node {
 	if len(nodeSets) == 1 {
+		if nodeSets[0].Len() == 0 {
+			return nil
+		}
 		nodeList := nodeSets[0].List()
 		return nodeList[len(nodeList)-1]
 	}

pkg/testmachinery/testflow/flow_operations_test.go

@@ -703,6 +703,7 @@ var serialTestDef = func() testdefinition.TestDefinition {
 		Template: &argov1.Template{},
 	}
 }()
+
 var defaultTestDef = testdefinition.NewEmpty()
 
 func createStepsFromNodes(nodes ...*node.Node) map[string]*testflow.Step {

pkg/testmachinery/testflow/flow_test.go

@@ -0,0 +1,67 @@
+package testflow_test
+
+import (
+	tmv1beta1 "github.com/gardener/test-infra/pkg/apis/testmachinery/v1beta1"
+	"github.com/gardener/test-infra/pkg/testmachinery/testdefinition"
+	"github.com/gardener/test-infra/pkg/testmachinery/testflow"
+	testutils "github.com/gardener/test-infra/test/utils"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("flow", func() {
+	Context("init", func() {
+		It("should set the root node as parent of all steps with no dependencies", func() {
+			rootNode := testNode("root", nil, defaultTestDef, nil)
+			locs := &testutils.LocationsMock{
+				StepToTestDefinitions: map[string][]*testdefinition.TestDefinition{
+					"create":        {testutils.TestDef("create")},
+					"delete":        {testutils.TestDef("delete")},
+					"tests-beta":    {testutils.TestDef("default"), testutils.SerialTestDef("serial"), testutils.DisruptiveTestDef("disruptive")},
+					"tests-release": {testutils.TestDef("default"), testutils.SerialTestDef("serial"), testutils.DisruptiveTestDef("disruptive")},
+				},
+			}
+			tf := tmv1beta1.TestFlow{}
+			Expect(testutils.ReadYAMLFile("./testdata/testflow-00.yaml", &tf)).To(Succeed())
+
+			_, err := testflow.NewFlow("testid", rootNode, tf, locs, nil)
+			Expect(err).ToNot(HaveOccurred())
+
+			Expect(rootNode.Children.Len()).To(Equal(1), "the root node should have one create child")
+			createNode := rootNode.Children.List()[0]
+			Expect(createNode.TestDefinition.Info.Name).To(Equal("create"))
+
+			// beta step
+			Expect(createNode.Children.Len()).To(Equal(1), "the create node should have 1 test child")
+			defaultBetaNode := createNode.Children.List()[0]
+			Expect(defaultBetaNode.TestDefinition.Info.Name).To(Equal("default"))
+
+			Expect(defaultBetaNode.Children.Len()).To(Equal(1), "the disruptive beta node should have 1 test child")
+			serialBetaNode := defaultBetaNode.Children.List()[0]
+			Expect(serialBetaNode.TestDefinition.Info.Name).To(Equal("serial"))
+
+			Expect(serialBetaNode.Children.Len()).To(Equal(1), "the default beta node should have 1 test child")
+			disruptiveBetaNode := serialBetaNode.Children.List()[0]
+			Expect(disruptiveBetaNode.TestDefinition.Info.Name).To(Equal("disruptive"))
+
+			// release step
+			Expect(disruptiveBetaNode.Children.Len()).To(Equal(1), "the disruptive beta node should have 1 test child")
+			defaultReleaseNode := disruptiveBetaNode.Children.List()[0]
+			Expect(defaultReleaseNode.TestDefinition.Info.Name).To(Equal("default"))
+
+			Expect(defaultReleaseNode.Children.Len()).To(Equal(1), "the default release node should have 1 test child")
+			serialReleaseNode := defaultReleaseNode.Children.List()[0]
+			Expect(serialReleaseNode.TestDefinition.Info.Name).To(Equal("serial"))
+
+			Expect(serialReleaseNode.Children.Len()).To(Equal(1), "the serial release node should have 1 test child")
+			disruptiveReleaseNode := serialReleaseNode.Children.List()[0]
+			Expect(disruptiveReleaseNode.TestDefinition.Info.Name).To(Equal("disruptive"))
+
+			// delete
+			Expect(disruptiveReleaseNode.Children.Len()).To(Equal(1), "the disruptive release node should have 1 test child")
+			deleteNode := disruptiveReleaseNode.Children.List()[0]
+			Expect(deleteNode.TestDefinition.Info.Name).To(Equal("delete"))
+		})
+	})
+})

pkg/testmachinery/testflow/node/node.go

@@ -79,13 +79,24 @@ func (n *Node) AddChildren(children ...*Node) {
 	n.Children.Add(children...)
 }
 
-// ClearParent removes a node from the current node's children
+// RemoveChild removes a node from the current node's children
 func (n *Node) RemoveChild(child *Node) {
+	child.RemoveParent(n)
 	n.Children.Remove(child)
 }
 
+// RemoveChildren removes all nodes from the current node's children
+func (n *Node) RemoveChildren(children ...*Node) {
+	for _, child := range children {
+		n.RemoveChild(child)
+	}
+}
+
 // ClearChildren removes all children from the current node
 func (n *Node) ClearChildren() {
+	for child := range n.Children.Iterate() {
+		child.RemoveParent(n)
+	}
 	n.Children = NewSet()
 }
 

pkg/testmachinery/testflow/node/node_suite_test.go

@@ -3,8 +3,6 @@ package node
 import (
 	"testing"
 
-	argov1 "github.com/argoproj/argo/pkg/apis/workflow/v1alpha1"
-
 	tmv1beta1 "github.com/gardener/test-infra/pkg/apis/testmachinery/v1beta1"
 	"github.com/gardener/test-infra/pkg/testmachinery/testdefinition"
 	testutils "github.com/gardener/test-infra/test/utils"
@@ -25,9 +23,9 @@ var _ = Describe("node operations", func() {
 			step.Definition.ContinueOnError = true
 			locs := &testutils.LocationsMock{
 				TestDefinitions: []*testdefinition.TestDefinition{
-					defaultTestDef,
-					serialTestDef(),
-					disruptiveTestDef(),
+					testutils.TestDef("default"),
+					testutils.SerialTestDef("serial"),
+					testutils.DisruptiveTestDef("disruptive"),
 				},
 			}
 			nodes, err := CreateNodesFromStep(step, locs, nil, "")
@@ -46,27 +44,3 @@ var _ = Describe("node operations", func() {
 
 	})
 })
-
-func serialTestDef() *testdefinition.TestDefinition {
-	return &testdefinition.TestDefinition{
-		Info: &tmv1beta1.TestDefinition{
-			Spec: tmv1beta1.TestDefSpec{
-				Behavior: []string{tmv1beta1.SerialBehavior},
-			},
-		},
-		Template: &argov1.Template{},
-	}
-}
-
-func disruptiveTestDef() *testdefinition.TestDefinition {
-	return &testdefinition.TestDefinition{
-		Info: &tmv1beta1.TestDefinition{
-			Spec: tmv1beta1.TestDefSpec{
-				Behavior: []string{tmv1beta1.DisruptiveBehavior},
-			},
-		},
-		Template: &argov1.Template{},
-	}
-}
-
-var defaultTestDef = testdefinition.NewEmpty()