correctly ensure deterministic spec order, even if specs are generate…

…d by iterating over a map

integration/_fixtures/nondeterministic_fixture/file_a_test.go

@@ -0,0 +1,27 @@
+package nondeterministic_fixture_test
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+)
+
+var cheat = func() {
+	It("in", func() {
+
+	})
+
+	It("order", func() {
+
+	})
+}
+
+var _ = Describe("ordered", Ordered, func() {
+	It("always", func() {
+
+	})
+
+	It("runs", func() {
+
+	})
+
+	cheat()
+})

integration/_fixtures/nondeterministic_fixture/file_b_test.go

@@ -0,0 +1,41 @@
+package nondeterministic_fixture_test
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+)
+
+var specGenerator = map[string]bool{
+	"map-A": true,
+	"map-B": true,
+	"map-C": true,
+	"map-D": true,
+	"map-E": true,
+	"map-F": true,
+	"map-G": true,
+	"map-H": true,
+	"map-I": true,
+	"map-J": true,
+}
+
+var _ = Describe("some tests, that include a test generated by iterating over a map", func() {
+	Describe("some tests", func() {
+		It("runs A", func() {
+
+		})
+		It("runs B", func() {
+
+		})
+	})
+
+	When("iterating over a map", func() {
+		for key := range specGenerator {
+			It("runs "+key, func() {
+
+			})
+		}
+	})
+
+	It("runs some other tests", func() {
+
+	})
+})

integration/_fixtures/nondeterministic_fixture/nondeterministic_fixture_suite_test.go

@@ -0,0 +1,36 @@
+package nondeterministic_fixture_test
+
+import (
+	"strings"
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	"github.com/onsi/ginkgo/v2/types"
+	. "github.com/onsi/gomega"
+)
+
+func TestNondeterministicFixture(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "NondeterministicFixture Suite")
+}
+
+var _ = ReportAfterSuite("ensure all specs ran correctly", func(report types.Report) {
+	specs := report.SpecReports.WithLeafNodeType(types.NodeTypeIt)
+	orderedTexts := []string{}
+	textCounts := map[string]int{}
+	for _, spec := range specs {
+		text := spec.FullText()
+		textCounts[text] += 1
+		if strings.HasPrefix(text, "ordered") {
+			orderedTexts = append(orderedTexts, spec.LeafNodeText)
+		}
+	}
+
+	By("ensuring there are no duplicates")
+	for text, count := range textCounts {
+		Ω(count).Should(Equal(1), text)
+	}
+
+	By("ensuring ordered specs are strictly preserved")
+	Ω(orderedTexts).Should(Equal([]string{"always", "runs", "in", "order"}))
+})

integration/run_test.go

@@ -444,6 +444,20 @@ var _ = Describe("Running Specs", func() {
 		})
 	})
 
+	Context("when running a suite in parallel that may have specs that are not deterministically ordered", func() {
+		BeforeEach(func() {
+			fm.MountFixture("nondeterministic")
+		})
+		It("successfully generates a stable sort across all parallel processes and ensures exactly the correct specs are run", func() {
+			By("Note: the assertions live in the ReportAfterSuite of the fixture.  So we simply assert that we succeeded")
+			session := startGinkgo(fm.PathTo("nondeterministic"), "--no-color", "--procs=3")
+			Eventually(session).Should(gexec.Exit(0))
+
+			session = startGinkgo(fm.PathTo("nondeterministic"), "--no-color", "--procs=3", "--randomize-all")
+			Eventually(session).Should(gexec.Exit(0))
+		})
+	})
+
 	Context("when there is a version mismatch between the cli and the test package", func() {
 		It("emits a useful error and tries running", func() {
 			fm.MountFixture(("version_mismatch"))

internal/ordering.go

@@ -26,6 +26,14 @@ func (s *SortableSpecs) Len() int      { return len(s.Indexes) }
 func (s *SortableSpecs) Swap(i, j int) { s.Indexes[i], s.Indexes[j] = s.Indexes[j], s.Indexes[i] }
 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
+	}
+
 	aCLs := a.Nodes.WithType(types.NodeTypesForContainerAndIt).CodeLocations()
 	bCLs := b.Nodes.WithType(types.NodeTypesForContainerAndIt).CodeLocations()
 	for i := 0; i < len(aCLs) && i < len(bCLs); i++ {
@@ -97,7 +105,6 @@ func OrderSpecs(specs Specs, suiteConfig types.SuiteConfig) (GroupedSpecIndices,
 	// we shuffle outermost containers.  so we need to form shufflable groupings of GroupIDs
 	shufflableGroupingIDs := []uint{}
 	shufflableGroupingIDToGroupIDs := map[uint][]uint{}
-	shufflableGroupingsIDToSortKeys := map[uint]string{}
 
 	// for each execution group we're going to have to pick a node to represent how the
 	// execution group is grouped for shuffling:
@@ -106,7 +113,7 @@ func OrderSpecs(specs Specs, suiteConfig types.SuiteConfig) (GroupedSpecIndices,
 		nodeTypesToShuffle = types.NodeTypeIt
 	}
 
-	//so, fo reach execution group:
+	//so, for each execution group:
 	for _, groupID := range executionGroupIDs {
 		// pick out a representative spec
 		representativeSpec := specs[executionGroups[groupID][0]]
@@ -121,22 +128,9 @@ func OrderSpecs(specs Specs, suiteConfig types.SuiteConfig) (GroupedSpecIndices,
 		if len(shufflableGroupingIDToGroupIDs[shufflableGroupingNode.ID]) == 1 {
 			// record the shuffleable group ID
 			shufflableGroupingIDs = append(shufflableGroupingIDs, shufflableGroupingNode.ID)
-			// and record the sort key to use
-			shufflableGroupingsIDToSortKeys[shufflableGroupingNode.ID] = shufflableGroupingNode.CodeLocation.String()
 		}
 	}
 
-	// now we sort the shufflable groups by the sort key.  We use the shufflable group nodes code location and break ties using its node id
-	sort.SliceStable(shufflableGroupingIDs, func(i, j int) bool {
-		keyA := shufflableGroupingsIDToSortKeys[shufflableGroupingIDs[i]]
-		keyB := shufflableGroupingsIDToSortKeys[shufflableGroupingIDs[j]]
-		if keyA == keyB {
-			return shufflableGroupingIDs[i] < shufflableGroupingIDs[j]
-		} else {
-			return keyA < keyB
-		}
-	})
-
 	// now we permute the sorted shufflable grouping IDs and build the ordered Groups
 	orderedGroups := GroupedSpecIndices{}
 	permutation := r.Perm(len(shufflableGroupingIDs))

internal/ordering_test.go

@@ -2,6 +2,7 @@ package internal_test
 
 import (
 	"strings"
+	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
@@ -131,7 +132,6 @@ var _ = Describe("OrderSpecs", func() {
 				S(con2, N("G", ntIt, CL("file_B", 15))),
 				S(con2, N("H", ntIt, CL("file_B", 20))),
 			}
-
 		})
 
 		It("always generates a consistent randomization when given the same seed", func() {
@@ -279,15 +279,16 @@ var _ = Describe("OrderSpecs", func() {
 		Describe("presorting-specs", func() {
 			BeforeEach(func() {
 				conA0 := N(ntCon, CL("file-A", 1))
-				conA1 := N(ntCon, CL("file-A", 4))
+				conA1 := N(ntCon, Ordered, CL("file-A", 4))
 				conA2 := N(ntCon, CL("file-A", 10))
 				conB0 := N(ntCon, CL("file-B", 1))
 				conC0 := N(ntCon, CL("file-C", 1))
 				specs = Specs{
 					S(conA0, N("A", ntIt, CL("file-A", 2))),
 					S(conA0, N("B", ntIt, CL("file-A", 3))),
-					S(conA0, conA1, N("C", ntIt, CL("file-A", 5))),
-					S(conA0, conA1, N("D", ntIt, CL("file-A", 6))),
+					// C and D are generated by a helper function in a different file.  if we aren't careful they would sort after E.  But conA1 is an Ordered container so its important things run in the correct order
+					S(conA0, conA1, N("C", ntIt, CL("file-Z", 100))),
+					S(conA0, conA1, N("D", ntIt, CL("file-Z", 99))),
 					S(conA0, conA1, N(ntCon, CL("file-A", 7)), N("E", ntIt, CL("file-A", 8))),
 					S(conA0, N("F", ntIt, CL("file-A", 9))),
 					S(conA0, conA2, N("G", ntIt, CL("file-A", 11))),
@@ -306,15 +307,59 @@ var _ = Describe("OrderSpecs", func() {
 				conf.RandomizeAllSpecs = false
 			})
 
-			It("ensures a deterministic order for specs that are defined at the same line without messing with the natural order of specs and containers", func() {
+			It("ensures a deterministic order for specs that are defined at the same line without messing with the natural order of specs and containers; it also ensures ordered containers run in the correct order - even if specs are generated in a helper function at a different line", func() {
 				conf.RandomSeed = 1 // this happens to sort conA0 ahead of conB0 - other than that, though, we are actually testing SortableSpecs
 				groupedSpecIndices, serialSpecIndices := internal.OrderSpecs(specs, conf)
 				Ω(serialSpecIndices).Should(BeEmpty())
 
 				Ω(getTexts(specs, groupedSpecIndices).Join()).Should(Equal("ABCDEFGHB-ZB-YB-BB-CB-DB-AC-AC-BC-CC-DC-EC-F"))
-
 			})
 		})
 
+		Describe("presorting-specs with randomize-all enabled", func() {
+			generateSpecs := func() Specs {
+				conA0 := N(ntCon, CL("file-A", 1))
+				conA1 := N(ntCon, CL("file-A", 4))
+				conA2 := N(ntCon, CL("file-A", 10))
+				conB0 := N(ntCon, CL("file-B", 1))
+				conC0 := N(ntCon, CL("file-C", 1))
+				specs := Specs{
+					S(conA0, N("A", ntIt, CL("file-A", 2))),
+					S(conA0, N("B", ntIt, CL("file-A", 3))),
+					S(conA0, conA1, N("C", ntIt, CL("file-A", 5))),
+					S(conA0, conA1, N("D", ntIt, CL("file-A", 6))),
+					S(conA0, conA1, N(ntCon, CL("file-A", 7)), N("E", ntIt, CL("file-A", 8))),
+					S(conA0, N("F", ntIt, CL("file-A", 9))),
+					S(conA0, conA2, N("G", ntIt, CL("file-A", 11))),
+					S(conA0, conA2, N("H", ntIt, CL("file-A", 12))),
+					S(conB0, N("B-Z", ntIt, CL("file-B", 2))),
+					S(conB0, N("B-Y", ntIt, CL("file-B", 3))),
+					S(conB0, N("B-D", ntIt, CL("file-B", 4))),
+					S(conB0, N("B-C", ntIt, CL("file-B", 4))),
+					S(conB0, N("B-B", ntIt, CL("file-B", 4))),
+					S(conB0, N("B-A", ntIt, CL("file-B", 5))),
+				}
+
+				for key := range map[string]bool{"C-A": true, "C-B": true, "C-C": true, "C-D": true, "C-E": true, "C-F": true} {
+					specs = append(specs, S(conC0, N(key, ntIt, CL("file-C", 2)))) // normally this would be totally non-deterministic
+				}
+				return specs
+			}
+
+			It("ensures a deterministic order for specs that are defined at the same line", func() {
+				conf.RandomSeed = time.Now().Unix()
+				conf.RandomizeAllSpecs = true
+
+				specsA := generateSpecs()
+				specsB := generateSpecs()
+				groupedSpecIndicesA, serialSpecIndices := internal.OrderSpecs(specsA, conf)
+				Ω(serialSpecIndices).Should(BeEmpty())
+				groupedSpecIndicesB, serialSpecIndices := internal.OrderSpecs(specsB, conf)
+				Ω(serialSpecIndices).Should(BeEmpty())
+
+				Ω(getTexts(specsA, groupedSpecIndicesA).Join()).Should(Equal(getTexts(specsB, groupedSpecIndicesB).Join()))
+
+			}, MustPassRepeatedly(5))
+		})
 	})
 })