Add tests for observabilitytest, change api to return error. (#482)

* Add tests for observabilitytest, change api to return error.

* Fix comment example.

* Remove debugging printfs and update test names for nop.

Author: Bogdan Drutu

exporter/exportertest/nop_exporter.go

@@ -21,17 +21,20 @@ import (
 	"github.com/census-instrumentation/opencensus-service/exporter"
 )
 
-type nopExporter int
+// NopExporterOption represents options that can be applied to a NopExporter.
+type NopExporterOption func(*nopExporter)
+
+type nopExporter struct{ retError error }
 
 var _ exporter.TraceExporter = (*nopExporter)(nil)
 var _ exporter.MetricsExporter = (*nopExporter)(nil)
 
 func (ne *nopExporter) ConsumeTraceData(ctx context.Context, td data.TraceData) error {
-	return nil
+	return ne.retError
 }
 
 func (ne *nopExporter) ConsumeMetricsData(ctx context.Context, md data.MetricsData) error {
-	return nil
+	return ne.retError
 }
 
 const (
@@ -48,11 +51,26 @@ func (ne *nopExporter) MetricsExportFormat() string {
 }
 
 // NewNopTraceExporter creates an TraceExporter that just drops the received data.
-func NewNopTraceExporter() exporter.TraceExporter {
-	return new(nopExporter)
+func NewNopTraceExporter(options ...NopExporterOption) exporter.TraceExporter {
+	return newNopExporter(options...)
 }
 
 // NewNopMetricsExporter creates an MetricsExporter that just drops the received data.
-func NewNopMetricsExporter() exporter.MetricsExporter {
-	return new(nopExporter)
+func NewNopMetricsExporter(options ...NopExporterOption) exporter.MetricsExporter {
+	return newNopExporter(options...)
+}
+
+// WithReturnError returns a NopExporterOption that enforces the nop Exporters to return the given error.
+func WithReturnError(retError error) NopExporterOption {
+	return func(ne *nopExporter) {
+		ne.retError = retError
+	}
+}
+
+func newNopExporter(options ...NopExporterOption) *nopExporter {
+	ne := new(nopExporter)
+	for _, opt := range options {
+		opt(ne)
+	}
+	return ne
 }

exporter/exportertest/nop_exporter_test.go

@@ -15,39 +15,64 @@ package exportertest
 
 import (
 	"context"
+	"errors"
 	"testing"
 
 	metricspb "github.com/census-instrumentation/opencensus-proto/gen-go/metrics/v1"
 	tracepb "github.com/census-instrumentation/opencensus-proto/gen-go/trace/v1"
 	"github.com/census-instrumentation/opencensus-service/data"
 )
 
-func TestNopTraceExporterNoErrors(t *testing.T) {
+func TestNopTraceExporter_NoErrors(t *testing.T) {
 	nte := NewNopTraceExporter()
 	td := data.TraceData{
 		Spans: make([]*tracepb.Span, 7),
 	}
 	if err := nte.ConsumeTraceData(context.Background(), td); err != nil {
-		t.Errorf("Wanted nil got error")
-		return
+		t.Fatalf("Wanted nil got error")
 	}
 	if "nop_trace" != nte.TraceExportFormat() {
-		t.Errorf("Wanted nop_trace got %s", nte.TraceExportFormat())
-		return
+		t.Fatalf("Wanted nop_trace got %s", nte.TraceExportFormat())
 	}
 }
 
-func TestNoopMetricsExporterNoErrors(t *testing.T) {
+func TestNopTraceExporter_WithErrors(t *testing.T) {
+	want := errors.New("MyError")
+	nte := NewNopTraceExporter(WithReturnError(want))
+	td := data.TraceData{
+		Spans: make([]*tracepb.Span, 7),
+	}
+	if got := nte.ConsumeTraceData(context.Background(), td); got != want {
+		t.Fatalf("Want %v Got %v", want, got)
+	}
+	if "nop_trace" != nte.TraceExportFormat() {
+		t.Fatalf("Wanted nop_trace got %s", nte.TraceExportFormat())
+	}
+}
+
+func TestNopMetricsExporter_NoErrors(t *testing.T) {
 	nme := NewNopMetricsExporter()
 	md := data.MetricsData{
 		Metrics: make([]*metricspb.Metric, 7),
 	}
 	if err := nme.ConsumeMetricsData(context.Background(), md); err != nil {
-		t.Errorf("Wanted nil got error")
-		return
+		t.Fatalf("Wanted nil got error")
+	}
+	if "nop_metrics" != nme.MetricsExportFormat() {
+		t.Fatalf("Wanted nop_metrics got %s", nme.MetricsExportFormat())
+	}
+}
+
+func TestNopMetricsExporter_WithErrors(t *testing.T) {
+	want := errors.New("MyError")
+	nme := NewNopMetricsExporter(WithReturnError(want))
+	md := data.MetricsData{
+		Metrics: make([]*metricspb.Metric, 7),
+	}
+	if got := nme.ConsumeMetricsData(context.Background(), md); got != want {
+		t.Fatalf("Want %v Got %v", want, got)
 	}
 	if "nop_metrics" != nme.MetricsExportFormat() {
-		t.Errorf("Wanted nop_metrics got %s", nme.MetricsExportFormat())
-		return
+		t.Fatalf("Wanted nop_metrics got %s", nme.MetricsExportFormat())
 	}
 }

exporter/exportertest/sink_exporter_test.go

@@ -31,8 +31,7 @@ func TestSinkTraceExporter(t *testing.T) {
 	want := make([]data.TraceData, 0, 7)
 	for i := 0; i < 7; i++ {
 		if err := sink.ConsumeTraceData(context.Background(), td); err != nil {
-			t.Errorf("Wanted nil got error")
-			return
+			t.Fatalf("Wanted nil got error")
 		}
 		want = append(want, td)
 	}
@@ -42,7 +41,6 @@ func TestSinkTraceExporter(t *testing.T) {
 	}
 	if "sink_trace" != sink.TraceExportFormat() {
 		t.Errorf("Wanted sink_trace got %s", sink.TraceExportFormat())
-		return
 	}
 }
 
@@ -54,8 +52,7 @@ func TestSinkMetricsExporter(t *testing.T) {
 	want := make([]data.MetricsData, 0, 7)
 	for i := 0; i < 7; i++ {
 		if err := sink.ConsumeMetricsData(context.Background(), md); err != nil {
-			t.Errorf("Wanted nil got error")
-			return
+			t.Fatalf("Wanted nil got error")
 		}
 		want = append(want, md)
 	}
@@ -65,6 +62,5 @@ func TestSinkMetricsExporter(t *testing.T) {
 	}
 	if "sink_metrics" != sink.MetricsExportFormat() {
 		t.Errorf("Wanted sink_metrics got %s", sink.MetricsExportFormat())
-		return
 	}
 }

exporter/loggingexporter/logging_exporter_test.go

@@ -30,8 +30,7 @@ func TestLoggingTraceExporterNoErrors(t *testing.T) {
 		Spans: make([]*tracepb.Span, 7),
 	}
 	if err := lte.ConsumeTraceData(context.Background(), td); err != nil {
-		t.Errorf("Wanted nil got error")
-		return
+		t.Fatalf("Wanted nil got error")
 	}
 	if "logging_trace" != lte.TraceExportFormat() {
 		t.Errorf("Wanted logging_trace got %v", lte.TraceExportFormat())
@@ -40,7 +39,9 @@ func TestLoggingTraceExporterNoErrors(t *testing.T) {
 
 func TestLoggingTraceExporterRecordMetrics(t *testing.T) {
 	lte := NewTraceExporter(zap.NewNop())
-	observabilitytest.CheckRecordedMetricsForTraceExporter(t, lte)
+	if err := observabilitytest.CheckRecordedMetricsForTraceExporter(lte); err != nil {
+		t.Fatalf("When test LoggingTraceExporter recording metrics: want nil got %v", err)
+	}
 }
 
 func TestLoggingMetricsExporterNoErrors(t *testing.T) {
@@ -49,8 +50,7 @@ func TestLoggingMetricsExporterNoErrors(t *testing.T) {
 		Metrics: make([]*metricspb.Metric, 7),
 	}
 	if err := lme.ConsumeMetricsData(context.Background(), md); err != nil {
-		t.Errorf("Wanted nil got error")
-		return
+		t.Fatalf("Wanted nil got error")
 	}
 	if "logging_metrics" != lme.MetricsExportFormat() {
 		t.Errorf("Wanted logging_metrics got %v", lme.MetricsExportFormat())

observability/observability_test.go

@@ -16,10 +16,6 @@
 // observability and observabilitytest
 package observability_test
 
-// This file contains helpers that are useful to add observability
-// with metrics and tracing using OpenCensus to the various pieces
-// of the service.
-
 import (
 	"context"
 	"testing"
@@ -34,14 +30,22 @@ const (
 )
 
 func TestTracePieplineRecordedMetrics(t *testing.T) {
-	defer observabilitytest.SetupRecordedMetricsTest(t)()
+	defer observabilitytest.SetupRecordedMetricsTest()()
 
 	receiverCtx := observability.ContextWithReceiverName(context.Background(), receiverName)
 	observability.RecordTraceReceiverMetrics(receiverCtx, 17, 13)
 	exporterCtx := observability.ContextWithExporterName(receiverCtx, exporterName)
 	observability.RecordTraceExporterMetrics(exporterCtx, 27, 23)
-	observabilitytest.CheckValueViewReceiverReceivedSpans(t, receiverName, 17)
-	observabilitytest.CheckValueViewReceiverDroppedSpans(t, receiverName, 13)
-	observabilitytest.CheckValueViewExporterReceivedSpans(t, receiverName, exporterName, 27)
-	observabilitytest.CheckValueViewExporterDroppedSpans(t, receiverName, exporterName, 23)
+	if err := observabilitytest.CheckValueViewReceiverReceivedSpans(receiverName, 17); err != nil {
+		t.Fatalf("When check recorded values: want nil got %v", err)
+	}
+	if err := observabilitytest.CheckValueViewReceiverDroppedSpans(receiverName, 13); err != nil {
+		t.Fatalf("When check recorded values: want nil got %v", err)
+	}
+	if err := observabilitytest.CheckValueViewExporterReceivedSpans(receiverName, exporterName, 27); err != nil {
+		t.Fatalf("When check recorded values: want nil got %v", err)
+	}
+	if err := observabilitytest.CheckValueViewExporterDroppedSpans(receiverName, exporterName, 23); err != nil {
+		t.Fatalf("When check recorded values: want nil got %v", err)
+	}
 }

observability/observabilitytest/observabilitytest.go

@@ -16,9 +16,9 @@ package observabilitytest
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 	"sort"
-	"testing"
 	"time"
 
 	tracepb "github.com/census-instrumentation/opencensus-proto/gen-go/trace/v1"
@@ -42,16 +42,14 @@ var _ view.Exporter = (*nopMetricsExporter)(nil)
 func (cme *nopMetricsExporter) ExportView(vd *view.Data) {}
 
 // SetupRecordedMetricsTest does setup the testing environment to check the metrics recorded by receivers, producers or exporters.
-// The returned function should be deferred "defer SetupRecordedMetricsTest(t)()".
-func SetupRecordedMetricsTest(t *testing.T) func() {
+// The returned function should be deferred "defer SetupRecordedMetricsTest()()".
+func SetupRecordedMetricsTest() func() {
 	// Register a nop metrics exporter for the OC library.
 	nmp := new(nopMetricsExporter)
 	view.RegisterExporter(nmp)
 
 	// Now for the stats exporter
-	if err := view.Register(observability.AllViews...); err != nil {
-		t.Fatalf("Failed to register all views: %v", err)
-	}
+	view.Register(observability.AllViews...)
 
 	return func() {
 		view.UnregisterExporter(nmp)
@@ -62,8 +60,8 @@ func SetupRecordedMetricsTest(t *testing.T) func() {
 // CheckRecordedMetricsForTraceExporter checks that the given TraceExporter records the correct set of metrics with the correct
 // set of tags by sending few TraceData to the exporter. The exporter should be able to handle the requests correctly without
 // dropping.
-func CheckRecordedMetricsForTraceExporter(t *testing.T, te exporter.TraceExporter) {
-	defer SetupRecordedMetricsTest(t)()
+func CheckRecordedMetricsForTraceExporter(te exporter.TraceExporter) error {
+	defer SetupRecordedMetricsTest()()
 
 	now := time.Now().UTC()
 	spans := []*tracepb.Span{
@@ -87,70 +85,64 @@ func CheckRecordedMetricsForTraceExporter(t *testing.T, te exporter.TraceExporte
 				},
 			},
 		},
-		{
-			TraceId:      []byte{0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28, 0x29, 0x2A, 0x2B, 0x2C, 0x2D, 0x2E},
-			SpanId:       []byte{0x38, 0x39, 0x3A, 0x3B, 0x3C, 0x3D, 0x3E, 0x3F},
-			ParentSpanId: []byte{0xF0, 0xF1, 0xF2, 0xF3, 0xF4, 0xF5, 0xF6, 0xF7},
-			Name:         &tracepb.TruncatableString{Value: "LocalSpan"},
-			Kind:         tracepb.Span_SPAN_KIND_UNSPECIFIED,
-			StartTime:    internal.TimeToTimestamp(now.Add(-15 * time.Millisecond)),
-			EndTime:      internal.TimeToTimestamp(now),
-			Status:       &tracepb.Status{Code: int32(0), Message: "OK"},
-			Tracestate:   &tracepb.Span_Tracestate{},
-		},
 	}
 	td := data.TraceData{Spans: spans}
 	ctx := observability.ContextWithReceiverName(context.Background(), fakeReceiverName)
 	const numBatches = 7
 	for i := 0; i < numBatches; i++ {
 		if err := te.ConsumeTraceData(ctx, td); err != nil {
-			t.Fatalf("Want nil got %v", err)
+			return fmt.Errorf("Want nil got %v", err)
 		}
 	}
 
-	CheckValueViewExporterReceivedSpans(t, fakeReceiverName, te.TraceExportFormat(), int64(numBatches*len(spans)))
-	CheckValueViewExporterDroppedSpans(t, fakeReceiverName, te.TraceExportFormat(), 0)
+	if err := CheckValueViewExporterReceivedSpans(fakeReceiverName, te.TraceExportFormat(), numBatches*len(spans)); err != nil {
+		return err
+	}
+	if err := CheckValueViewExporterDroppedSpans(fakeReceiverName, te.TraceExportFormat(), 0); err != nil {
+		return err
+	}
+	return nil
 }
 
 // CheckValueViewExporterReceivedSpans checks that for the current exported value in the ViewExporterReceivedSpans
 // for {TagKeyReceiver: receiverName, TagKeyExporter: exporterTagName} is equal to "value".
-// In tests that this function is called it is required to also call SetupRecordedMetricsTest as first thing.
-func CheckValueViewExporterReceivedSpans(t *testing.T, receiverName string, exporterTagName string, value int64) {
-	checkValueForView(t, observability.ViewExporterReceivedSpans.Name,
-		wantsTagsForExporterView(receiverName, exporterTagName), value)
+// When this function is called it is required to also call SetupRecordedMetricsTest as first thing.
+func CheckValueViewExporterReceivedSpans(receiverName string, exporterTagName string, value int) error {
+	return checkValueForView(observability.ViewExporterReceivedSpans.Name,
+		wantsTagsForExporterView(receiverName, exporterTagName), int64(value))
 }
 
 // CheckValueViewExporterDroppedSpans checks that for the current exported value in the ViewExporterDroppedSpans
 // for {TagKeyReceiver: receiverName} is equal to "value".
 // In tests that this function is called it is required to also call SetupRecordedMetricsTest as first thing.
-func CheckValueViewExporterDroppedSpans(t *testing.T, receiverName string, exporterTagName string, value int64) {
-	checkValueForView(t, observability.ViewExporterDroppedSpans.Name,
-		wantsTagsForExporterView(receiverName, exporterTagName), value)
+func CheckValueViewExporterDroppedSpans(receiverName string, exporterTagName string, value int) error {
+	return checkValueForView(observability.ViewExporterDroppedSpans.Name,
+		wantsTagsForExporterView(receiverName, exporterTagName), int64(value))
 }
 
 // CheckValueViewReceiverReceivedSpans checks that for the current exported value in the ViewReceiverReceivedSpans
 // for {TagKeyReceiver: receiverName, TagKeyExporter: exporterTagName} is equal to "value".
 // In tests that this function is called it is required to also call SetupRecordedMetricsTest as first thing.
-func CheckValueViewReceiverReceivedSpans(t *testing.T, receiverName string, value int64) {
-	checkValueForView(t, observability.ViewReceiverReceivedSpans.Name,
-		wantsTagsForReceiverView(receiverName), value)
+func CheckValueViewReceiverReceivedSpans(receiverName string, value int) error {
+	return checkValueForView(observability.ViewReceiverReceivedSpans.Name,
+		wantsTagsForReceiverView(receiverName), int64(value))
 }
 
 // CheckValueViewReceiverDroppedSpans checks that for the current exported value in the ViewReceiverDroppedSpans
 // for {TagKeyReceiver: receiverName} is equal to "value".
 // In tests that this function is called it is required to also call SetupRecordedMetricsTest as first thing.
-func CheckValueViewReceiverDroppedSpans(t *testing.T, receiverName string, value int64) {
-	checkValueForView(t, observability.ViewReceiverDroppedSpans.Name,
-		wantsTagsForReceiverView(receiverName), value)
+func CheckValueViewReceiverDroppedSpans(receiverName string, value int) error {
+	return checkValueForView(observability.ViewReceiverDroppedSpans.Name,
+		wantsTagsForReceiverView(receiverName), int64(value))
 }
 
-func checkValueForView(t *testing.T, vName string, wantTags []tag.Tag, value int64) {
+func checkValueForView(vName string, wantTags []tag.Tag, value int64) error {
 	// Make sure the tags slice is sorted by tag keys.
 	sortTags(wantTags)
 
 	rows, err := view.RetrieveData(vName)
 	if err != nil {
-		t.Fatalf("Error retrieving view data for view Name %s", vName)
+		return fmt.Errorf("Error retrieving view data for view Name %s", vName)
 	}
 
 	for _, row := range rows {
@@ -159,13 +151,13 @@ func checkValueForView(t *testing.T, vName string, wantTags []tag.Tag, value int
 		if reflect.DeepEqual(wantTags, row.Tags) {
 			sum := row.Data.(*view.SumData)
 			if float64(value) != sum.Value {
-				t.Fatalf("Want %v got %v", float64(value), sum.Value)
+				return fmt.Errorf("Different recorded value: want %v got %v", float64(value), sum.Value)
 			}
 			// We found the result
-			return
+			return nil
 		}
 	}
-	t.Fatalf("Could not find wantTags: %s in rows %v", wantTags, rows)
+	return fmt.Errorf("Could not find wantTags: %s in rows %v", wantTags, rows)
 }
 
 func wantsTagsForExporterView(receiverName string, exporterTagName string) []tag.Tag {