ensure the correct deterministic sort order is produced when ordered specs are generated by a helper function

fixes a subtle bug described in #1198 - the sort order in Ordering.go was not generating a consistent order for Ordered specs generated by helper functions

Author: onsi

internal/internal_integration/ordered_test.go

@@ -1281,4 +1281,55 @@ var _ = DescribeTable("Ordered Containers",
 	},
 		"A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", HavePassed(),
 	),
+
+	// Bug reported in #1198
+	Entry("bug reported in #1198 - probably due to attempts to ensure deterministic order", true, func() {
+		Describe("outer", func() { // the only non-Ordered Describe
+			OrderedRun("a")
+			OrderedRun("b")
+			OrderedRun("c")
+		})
+	}, []string{"a1", "a2", "a3", "a4", "a5", "b1", "b2", "b3", "b4", "b5", "c1", "c2", "c3", "c4", "c5"}),
 )
+
+// OrderedRun creates a sequence of tests, all are intended to be Ordered - this is the reproducer provided in #1198
+func OrderedRun(description string) {
+	Describe(description, Ordered, func() {
+		It(fmt.Sprint(description, "1"), func() {
+			rt.Run(description + "1")
+		})
+
+		Sub2(description)
+		Sub3(description)
+
+		Describe("inner", func() {
+			Sub4(description)
+		})
+	})
+}
+
+func Sub2(description string) {
+	It(fmt.Sprint(description, "2"), func() {
+		rt.Run(description + "2")
+	})
+}
+
+func Sub3(description string) {
+	It(fmt.Sprint(description, "3"), func() {
+		rt.Run(description + "3")
+	})
+}
+
+func Sub4(description string) {
+	Describe("deeply nested 1", func() {
+		It(fmt.Sprint(description, "4"), func() {
+			rt.Run(description + "4")
+		})
+	})
+
+	Describe("deeply nested 2", func() {
+		It(fmt.Sprint(description, "5"), func() {
+			rt.Run(description + "5")
+		})
+	})
+}

internal/node.go

@@ -875,6 +875,15 @@ func (n Nodes) FirstNodeMarkedOrdered() Node {
 	return Node{}
 }
 
+func (n Nodes) IndexOfFirstNodeMarkedOrdered() int {
+	for i := range n {
+		if n[i].MarkedOrdered {
+			return i
+		}
+	}
+	return -1
+}
+
 func (n Nodes) GetMaxFlakeAttempts() int {
 	maxFlakeAttempts := 0
 	for i := range n {

internal/node_test.go

@@ -1579,6 +1579,22 @@ var _ = Describe("Nodes", func() {
 		})
 	})
 
+	Describe("IndexOfFirstNodeMarkedOrdered", func() {
+		Context("when there are nodes marked ordered", func() {
+			It("returns the index of the first one", func() {
+				nodes := Nodes{N(), N("A", ntCon, Ordered), N("B", ntCon, Ordered), N()}
+				Ω(nodes.IndexOfFirstNodeMarkedOrdered()).Should(Equal(1))
+			})
+		})
+
+		Context("when there is no node marked ordered", func() {
+			It("returns -1", func() {
+				nodes := Nodes{N(), N(), N()}
+				Ω(nodes.IndexOfFirstNodeMarkedOrdered()).Should(Equal(-1))
+			})
+		})
+	})
+
 	Describe("GetMaxFlakeAttempts", func() {
 		Context("when there is no node marked with FlakeAttempts decorator", func() {
 			It("returns 0", func() {

internal/ordering.go

@@ -27,36 +27,43 @@ func (s *SortableSpecs) Swap(i, j int) { s.Indexes[i], s.Indexes[j] = s.Indexes[
 func (s *SortableSpecs) Less(i, j int) bool {
 	a, b := s.Specs[s.Indexes[i]], s.Specs[s.Indexes[j]]
 
-	firstOrderedA := a.Nodes.FirstNodeMarkedOrdered()
-	firstOrderedB := b.Nodes.FirstNodeMarkedOrdered()
-	if firstOrderedA.ID == firstOrderedB.ID && !firstOrderedA.IsZero() {
-		// strictly preserve order in ordered containers.  ID will track this as IDs are generated monotonically
-		return a.FirstNodeWithType(types.NodeTypeIt).ID < b.FirstNodeWithType(types.NodeTypeIt).ID
+	aNodes, bNodes := a.Nodes.WithType(types.NodeTypesForContainerAndIt), b.Nodes.WithType(types.NodeTypesForContainerAndIt)
+
+	firstOrderedAIdx, firstOrderedBIdx := aNodes.IndexOfFirstNodeMarkedOrdered(), bNodes.IndexOfFirstNodeMarkedOrdered()
+	if firstOrderedAIdx > -1 && firstOrderedBIdx > -1 && aNodes[firstOrderedAIdx].ID == bNodes[firstOrderedBIdx].ID {
+		// strictly preserve order within an ordered containers.  ID will track this as IDs are generated monotonically
+		return aNodes.FirstNodeWithType(types.NodeTypeIt).ID < bNodes.FirstNodeWithType(types.NodeTypeIt).ID
+	}
+
+	// if either spec is in an ordered container - only use the nodes up to the outermost ordered container
+	if firstOrderedAIdx > -1 {
+		aNodes = aNodes[:firstOrderedAIdx+1]
+	}
+	if firstOrderedBIdx > -1 {
+		bNodes = bNodes[:firstOrderedBIdx+1]
 	}
 
-	aCLs := a.Nodes.WithType(types.NodeTypesForContainerAndIt).CodeLocations()
-	bCLs := b.Nodes.WithType(types.NodeTypesForContainerAndIt).CodeLocations()
-	for i := 0; i < len(aCLs) && i < len(bCLs); i++ {
-		aCL, bCL := aCLs[i], bCLs[i]
-		if aCL.FileName < bCL.FileName {
-			return true
-		} else if aCL.FileName > bCL.FileName {
-			return false
+	for i := 0; i < len(aNodes) && i < len(bNodes); i++ {
+		aCL, bCL := aNodes[i].CodeLocation, bNodes[i].CodeLocation
+		if aCL.FileName != bCL.FileName {
+			return aCL.FileName < bCL.FileName
 		}
-		if aCL.LineNumber < bCL.LineNumber {
-			return true
-		} else if aCL.LineNumber > bCL.LineNumber {
-			return false
+		if aCL.LineNumber != bCL.LineNumber {
+			return aCL.LineNumber < bCL.LineNumber
 		}
 	}
 	// either everything is equal or we have different lengths of CLs
-	if len(aCLs) < len(bCLs) {
-		return true
-	} else if len(aCLs) > len(bCLs) {
-		return false
+	if len(aNodes) != len(bNodes) {
+		return len(aNodes) < len(bNodes)
 	}
 	// ok, now we are sure everything was equal. so we use the spec text to break ties
-	return a.Text() < b.Text()
+	for i := 0; i < len(aNodes); i++ {
+		if aNodes[i].Text != bNodes[i].Text {
+			return aNodes[i].Text < bNodes[i].Text
+		}
+	}
+	// ok, all those texts were equal.  we'll use the ID of the most deeply nested node as a last resort
+	return aNodes[len(aNodes)-1].ID < bNodes[len(bNodes)-1].ID
 }
 
 type GroupedSpecIndices []SpecIndices