handle failures and panics in attached progress reporters

onsi · Mar 3, 2023 · ae8c900 · ae8c900
1 parent b670c81
commit ae8c900
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 21 deletions.
diff --git a/internal/internal_integration/progress_report_test.go b/internal/internal_integration/progress_report_test.go
@@ -4,6 +4,7 @@ import (
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/ginkgo/v2/internal/test_helpers"
 	"github.com/onsi/ginkgo/v2/types"
 	. "github.com/onsi/gomega"
 )
@@ -478,4 +479,26 @@ var _ = Describe("Progress Reporting", func() {
 			Ω(pr.AdditionalReports).Should(Equal([]string{"Some More (Never Cancelled) Global Information"}))
 		})
 	})
+
+	Context("when a global progress reporter fails", func() {
+		BeforeEach(func() {
+			success, _ := RunFixture("emitting spec progress", func() {
+				Describe("a container", func() {
+					It("A", func(ctx SpecContext) {
+						AttachProgressReporter(func() string {
+							F("bam")
+							return "Some Global Information"
+						})
+						triggerProgressSignal()
+					})
+				})
+			})
+			Ω(success).Should(BeFalse())
+		})
+
+		It("marks the spec as failed", func() {
+			Ω(reporter.Did.Find("A")).Should(HaveFailed("bam"))
+		})
+	})
+
 })
diff --git a/internal/progress_reporter_manager.go b/internal/progress_reporter_manager.go
@@ -4,6 +4,8 @@ import (
 	"context"
 	"sort"
 	"sync"
+
+	"github.com/onsi/ginkgo/v2/types"
 )
 
 type ProgressReporterManager struct {
@@ -33,7 +35,7 @@ func (prm *ProgressReporterManager) AttachProgressReporter(reporter func() strin
 	}
 }
 
-func (prm *ProgressReporterManager) QueryProgressReporters(ctx context.Context) []string {
+func (prm *ProgressReporterManager) QueryProgressReporters(ctx context.Context, failer *Failer) []string {
 	prm.lock.Lock()
 	keys := []int{}
 	for key := range prm.progressReporters {
@@ -53,6 +55,13 @@ func (prm *ProgressReporterManager) QueryProgressReporters(ctx context.Context)
 	for _, reporter := range reporters {
 		reportC := make(chan string, 1)
 		go func() {
+			defer func() {
+				e := recover()
+				if e != nil {
+					failer.Panic(types.NewCodeLocationWithStackTrace(1), e)
+					reportC <- "failed to query attached progress reporter"
+				}
+			}()
 			reportC <- reporter()
 		}()
 		var report string

diff --git a/internal/progress_reporter_manager_test.go b/internal/progress_reporter_manager_test.go
@@ -5,6 +5,7 @@ import (
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
+	"github.com/onsi/ginkgo/v2/types"
 	. "github.com/onsi/gomega"
 	"github.com/onsi/gomega/gleak"
 
@@ -19,19 +20,19 @@ var _ = Describe("ProgressReporterManager", func() {
 	})
 
 	It("can attach and detach progress reporters", func() {
-		Ω(manager.QueryProgressReporters(context.Background())).Should(BeEmpty())
+		Ω(manager.QueryProgressReporters(context.Background(), nil)).Should(BeEmpty())
 		cancelA := manager.AttachProgressReporter(func() string { return "A" })
-		Ω(manager.QueryProgressReporters(context.Background())).Should(Equal([]string{"A"}))
+		Ω(manager.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"A"}))
 		cancelB := manager.AttachProgressReporter(func() string { return "B" })
-		Ω(manager.QueryProgressReporters(context.Background())).Should(Equal([]string{"A", "B"}))
+		Ω(manager.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"A", "B"}))
 		cancelC := manager.AttachProgressReporter(func() string { return "C" })
-		Ω(manager.QueryProgressReporters(context.Background())).Should(Equal([]string{"A", "B", "C"}))
+		Ω(manager.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"A", "B", "C"}))
 		cancelB()
-		Ω(manager.QueryProgressReporters(context.Background())).Should(Equal([]string{"A", "C"}))
+		Ω(manager.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"A", "C"}))
 		cancelA()
-		Ω(manager.QueryProgressReporters(context.Background())).Should(Equal([]string{"C"}))
+		Ω(manager.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"C"}))
 		cancelC()
-		Ω(manager.QueryProgressReporters(context.Background())).Should(BeEmpty())
+		Ω(manager.QueryProgressReporters(context.Background(), nil)).Should(BeEmpty())
 	})
 
 	It("bails if a progress reporter takes longer than the passed-in context's deadline", func() {
@@ -45,11 +46,25 @@ var _ = Describe("ProgressReporterManager", func() {
 		})
 		manager.AttachProgressReporter(func() string { return "D" })
 		context, cancel := context.WithTimeout(context.Background(), time.Millisecond*100)
-		result := manager.QueryProgressReporters(context)
+		result := manager.QueryProgressReporters(context, nil)
 		Ω(result).Should(Equal([]string{"A", "B"}))
 		cancel()
 		close(c)
 
 		Eventually(gleak.Goroutines).ShouldNot(gleak.HaveLeaked(startingGoroutines))
 	})
+
+	It("catches panics and reports them as failures", func() {
+		manager.AttachProgressReporter(func() string {
+			panic("bam")
+		})
+		manager.AttachProgressReporter(func() string { return "B" })
+		failer := internal.NewFailer()
+		result := manager.QueryProgressReporters(context.Background(), failer)
+		Ω(result).Should(Equal([]string{"failed to query attached progress reporter", "B"}))
+		state, failure := failer.Drain()
+		Ω(state).Should(Equal(types.SpecStatePanicked))
+		Ω(failure.Message).Should(Equal("Test Panicked"))
+		Ω(failure.ForwardedPanic).Should(Equal("bam"))
+	})
 })
diff --git a/internal/spec_context_test.go b/internal/spec_context_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	. "github.com/onsi/ginkgo/v2"
+	"github.com/onsi/ginkgo/v2/internal"
 	. "github.com/onsi/gomega"
 )
 
@@ -12,38 +13,38 @@ var _ = Describe("SpecContext", func() {
 		Ω(c.SpecReport().LeafNodeText).Should(Equal("allows access to the current spec report"))
 	})
 
-	It("can be wrapped and still retreived", func(c SpecContext) {
+	It("can be wrapped and still retrieved", func(c SpecContext) {
 		Ω(c.Value("GINKGO_SPEC_CONTEXT")).Should(Equal(c))
 
 		wrappedC := context.WithValue(c, "foo", "bar")
 
 		_, ok := wrappedC.(SpecContext)
 		Ω(ok).Should(BeFalse())
-		Ω(wrappedC.Value("GINKGO_SPEC_CONTEXT").(SpecContext).SpecReport().LeafNodeText).Should(Equal("can be wrapped and still retreived"))
+		Ω(wrappedC.Value("GINKGO_SPEC_CONTEXT").(SpecContext).SpecReport().LeafNodeText).Should(Equal("can be wrapped and still retrieved"))
 	})
 
 	It("can attach and detach progress reporters", func(c SpecContext) {
 		type CompleteSpecContext interface {
 			AttachProgressReporter(func() string) func()
-			QueryProgressReporters(ctx context.Context) []string
+			QueryProgressReporters(ctx context.Context, failer *internal.Failer) []string
 		}
 
 		wrappedC := context.WithValue(c, "foo", "bar")
 		ctx := wrappedC.Value("GINKGO_SPEC_CONTEXT").(CompleteSpecContext)
 
-		Ω(ctx.QueryProgressReporters(context.Background())).Should(BeEmpty())
+		Ω(ctx.QueryProgressReporters(context.Background(), nil)).Should(BeEmpty())
 
 		cancelA := ctx.AttachProgressReporter(func() string { return "A" })
-		Ω(ctx.QueryProgressReporters(context.Background())).Should(Equal([]string{"A"}))
+		Ω(ctx.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"A"}))
 		cancelB := ctx.AttachProgressReporter(func() string { return "B" })
-		Ω(ctx.QueryProgressReporters(context.Background())).Should(Equal([]string{"A", "B"}))
+		Ω(ctx.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"A", "B"}))
 		cancelC := ctx.AttachProgressReporter(func() string { return "C" })
-		Ω(ctx.QueryProgressReporters(context.Background())).Should(Equal([]string{"A", "B", "C"}))
+		Ω(ctx.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"A", "B", "C"}))
 		cancelB()
-		Ω(ctx.QueryProgressReporters(context.Background())).Should(Equal([]string{"A", "C"}))
+		Ω(ctx.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"A", "C"}))
 		cancelA()
-		Ω(ctx.QueryProgressReporters(context.Background())).Should(Equal([]string{"C"}))
+		Ω(ctx.QueryProgressReporters(context.Background(), nil)).Should(Equal([]string{"C"}))
 		cancelC()
-		Ω(ctx.QueryProgressReporters(context.Background())).Should(BeEmpty())
+		Ω(ctx.QueryProgressReporters(context.Background(), nil)).Should(BeEmpty())
 	})
 })
diff --git a/internal/suite.go b/internal/suite.go
@@ -348,9 +348,9 @@ func (suite *Suite) generateProgressReport(fullReport bool) types.ProgressReport
 	defer cancel()
 	var additionalReports []string
 	if suite.currentSpecContext != nil {
-		additionalReports = append(additionalReports, suite.currentSpecContext.QueryProgressReporters(deadline)...)
+		additionalReports = append(additionalReports, suite.currentSpecContext.QueryProgressReporters(deadline, suite.failer)...)
 	}
-	additionalReports = append(additionalReports, suite.QueryProgressReporters(deadline)...)
+	additionalReports = append(additionalReports, suite.QueryProgressReporters(deadline, suite.failer)...)
 	gwOutput := suite.currentSpecReport.CapturedGinkgoWriterOutput + string(suite.writer.Bytes())
 	pr, err := NewProgressReport(suite.isRunningInParallel(), suite.currentSpecReport, suite.currentNode, suite.currentNodeStartTime, suite.currentByStep, gwOutput, timelineLocation, additionalReports, suite.config.SourceRoots, fullReport)