styleguide: tests goroutine leaks and naming (#4348)

* internal/global: Fix goroutine leaks in tests

CONTRIBUTING.md

@@ -556,6 +556,13 @@ functionality should be added, each one will need their own super-set
 interfaces and will duplicate the pattern. For this reason, the simple targeted
 interface that defines the specific functionality should be preferred.
 
+### Testing
+
+The tests should never leak goroutines.
+
+Use the term `ConcurrentSafe` in the test name when it aims to verify the
+absence of race conditions.
+
 ## Approvers and Maintainers
 
 ### Approvers

exporters/otlp/internal/retry/retry_test.go

@@ -227,7 +227,7 @@ func TestRetryNotEnabled(t *testing.T) {
 	}), assert.AnError)
 }
 
-func TestConcurrentRetry(t *testing.T) {
+func TestRetryConcurrentSafe(t *testing.T) {
 	ev := func(error) (bool, time.Duration) { return true, 0 }
 	reqFunc := Config{
 		Enabled: true,

exporters/otlp/otlpmetric/internal/exporter_test.go

@@ -56,7 +56,7 @@ func (c *client) Shutdown(context.Context) error {
 	return nil
 }
 
-func TestExporterClientConcurrency(t *testing.T) {
+func TestExporterClientConcurrentSafe(t *testing.T) {
 	const goroutines = 5
 
 	exp := New(&client{})

exporters/otlp/otlptrace/otlptracehttp/client_test.go

@@ -330,7 +330,7 @@ func TestDeadlineContext(t *testing.T) {
 	assert.Empty(t, mc.GetSpans())
 }
 
-func TestStopWhileExporting(t *testing.T) {
+func TestStopWhileExportingConcurrentSafe(t *testing.T) {
 	statuses := make([]int, 0, 5)
 	for i := 0; i < cap(statuses); i++ {
 		statuses = append(statuses, http.StatusTooManyRequests)

internal/global/handler_test.go

@@ -19,7 +19,7 @@ import (
 	"errors"
 	"io"
 	"log"
-	"os"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/suite"
@@ -123,9 +123,24 @@ func TestHandlerTestSuite(t *testing.T) {
 	suite.Run(t, new(HandlerTestSuite))
 }
 
-func TestHandlerRace(t *testing.T) {
-	go SetErrorHandler(&ErrLogger{log.New(os.Stderr, "", 0)})
-	go Handle(errors.New("error"))
+func TestHandlerConcurrentSafe(t *testing.T) {
+	// In order not to pollute the test output.
+	SetErrorHandler(&ErrLogger{log.New(io.Discard, "", 0)})
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		SetErrorHandler(&ErrLogger{log.New(io.Discard, "", 0)})
+	}()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		Handle(errors.New("error"))
+	}()
+
+	wg.Wait()
+	reset()
 }
 
 func BenchmarkErrorHandler(b *testing.B) {

internal/global/instruments_test.go

@@ -23,9 +23,11 @@ import (
 	"go.opentelemetry.io/otel/metric/noop"
 )
 
-func testFloat64Race(interact func(float64), setDelegate func(metric.Meter)) {
+func testFloat64ConcurrentSafe(interact func(float64), setDelegate func(metric.Meter)) {
+	done := make(chan struct{})
 	finish := make(chan struct{})
 	go func() {
+		defer close(done)
 		for {
 			interact(1)
 			select {
@@ -38,11 +40,14 @@ func testFloat64Race(interact func(float64), setDelegate func(metric.Meter)) {
 
 	setDelegate(noop.NewMeterProvider().Meter(""))
 	close(finish)
+	<-done
 }
 
-func testInt64Race(interact func(int64), setDelegate func(metric.Meter)) {
+func testInt64ConcurrentSafe(interact func(int64), setDelegate func(metric.Meter)) {
+	done := make(chan struct{})
 	finish := make(chan struct{})
 	go func() {
+		defer close(done)
 		for {
 			interact(1)
 			select {
@@ -55,27 +60,28 @@ func testInt64Race(interact func(int64), setDelegate func(metric.Meter)) {
 
 	setDelegate(noop.NewMeterProvider().Meter(""))
 	close(finish)
+	<-done
 }
 
-func TestAsyncInstrumentSetDelegateRace(t *testing.T) {
+func TestAsyncInstrumentSetDelegateConcurrentSafe(t *testing.T) {
 	// Float64 Instruments
 	t.Run("Float64", func(t *testing.T) {
 		t.Run("Counter", func(t *testing.T) {
 			delegate := &afCounter{}
 			f := func(float64) { _ = delegate.Unwrap() }
-			testFloat64Race(f, delegate.setDelegate)
+			testFloat64ConcurrentSafe(f, delegate.setDelegate)
 		})
 
 		t.Run("UpDownCounter", func(t *testing.T) {
 			delegate := &afUpDownCounter{}
 			f := func(float64) { _ = delegate.Unwrap() }
-			testFloat64Race(f, delegate.setDelegate)
+			testFloat64ConcurrentSafe(f, delegate.setDelegate)
 		})
 
 		t.Run("Gauge", func(t *testing.T) {
 			delegate := &afGauge{}
 			f := func(float64) { _ = delegate.Unwrap() }
-			testFloat64Race(f, delegate.setDelegate)
+			testFloat64ConcurrentSafe(f, delegate.setDelegate)
 		})
 	})
 
@@ -85,42 +91,42 @@ func TestAsyncInstrumentSetDelegateRace(t *testing.T) {
 		t.Run("Counter", func(t *testing.T) {
 			delegate := &aiCounter{}
 			f := func(int64) { _ = delegate.Unwrap() }
-			testInt64Race(f, delegate.setDelegate)
+			testInt64ConcurrentSafe(f, delegate.setDelegate)
 		})
 
 		t.Run("UpDownCounter", func(t *testing.T) {
 			delegate := &aiUpDownCounter{}
 			f := func(int64) { _ = delegate.Unwrap() }
-			testInt64Race(f, delegate.setDelegate)
+			testInt64ConcurrentSafe(f, delegate.setDelegate)
 		})
 
 		t.Run("Gauge", func(t *testing.T) {
 			delegate := &aiGauge{}
 			f := func(int64) { _ = delegate.Unwrap() }
-			testInt64Race(f, delegate.setDelegate)
+			testInt64ConcurrentSafe(f, delegate.setDelegate)
 		})
 	})
 }
 
-func TestSyncInstrumentSetDelegateRace(t *testing.T) {
+func TestSyncInstrumentSetDelegateConcurrentSafe(t *testing.T) {
 	// Float64 Instruments
 	t.Run("Float64", func(t *testing.T) {
 		t.Run("Counter", func(t *testing.T) {
 			delegate := &sfCounter{}
 			f := func(v float64) { delegate.Add(context.Background(), v) }
-			testFloat64Race(f, delegate.setDelegate)
+			testFloat64ConcurrentSafe(f, delegate.setDelegate)
 		})
 
 		t.Run("UpDownCounter", func(t *testing.T) {
 			delegate := &sfUpDownCounter{}
 			f := func(v float64) { delegate.Add(context.Background(), v) }
-			testFloat64Race(f, delegate.setDelegate)
+			testFloat64ConcurrentSafe(f, delegate.setDelegate)
 		})
 
 		t.Run("Histogram", func(t *testing.T) {
 			delegate := &sfHistogram{}
 			f := func(v float64) { delegate.Record(context.Background(), v) }
-			testFloat64Race(f, delegate.setDelegate)
+			testFloat64ConcurrentSafe(f, delegate.setDelegate)
 		})
 	})
 
@@ -130,19 +136,19 @@ func TestSyncInstrumentSetDelegateRace(t *testing.T) {
 		t.Run("Counter", func(t *testing.T) {
 			delegate := &siCounter{}
 			f := func(v int64) { delegate.Add(context.Background(), v) }
-			testInt64Race(f, delegate.setDelegate)
+			testInt64ConcurrentSafe(f, delegate.setDelegate)
 		})
 
 		t.Run("UpDownCounter", func(t *testing.T) {
 			delegate := &siUpDownCounter{}
 			f := func(v int64) { delegate.Add(context.Background(), v) }
-			testInt64Race(f, delegate.setDelegate)
+			testInt64ConcurrentSafe(f, delegate.setDelegate)
 		})
 
 		t.Run("Histogram", func(t *testing.T) {
 			delegate := &siHistogram{}
 			f := func(v int64) { delegate.Record(context.Background(), v) }
-			testInt64Race(f, delegate.setDelegate)
+			testInt64ConcurrentSafe(f, delegate.setDelegate)
 		})
 	})
 }

internal/global/internal_logging_test.go

@@ -17,8 +17,9 @@ package global
 import (
 	"bytes"
 	"errors"
+	"io"
 	"log"
-	"os"
+	"sync"
 	"testing"
 
 	"github.com/go-logr/logr"
@@ -29,9 +30,21 @@ import (
 	"github.com/go-logr/stdr"
 )
 
-func TestRace(t *testing.T) {
-	go SetLogger(stdr.New(log.New(os.Stderr, "", 0)))
-	go Info("")
+func TestLoggerConcurrentSafe(t *testing.T) {
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		SetLogger(stdr.New(log.New(io.Discard, "", 0)))
+	}()
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		Info("")
+	}()
+
+	wg.Wait()
+	reset()
 }
 
 func TestLogLevel(t *testing.T) {

internal/global/meter_test.go

@@ -27,10 +27,12 @@ import (
 	"go.opentelemetry.io/otel/metric/noop"
 )
 
-func TestMeterProviderRace(t *testing.T) {
+func TestMeterProviderConcurrentSafe(t *testing.T) {
 	mp := &meterProvider{}
+	done := make(chan struct{})
 	finish := make(chan struct{})
 	go func() {
+		defer close(done)
 		for i := 0; ; i++ {
 			mp.Meter(fmt.Sprintf("a%d", i))
 			select {
@@ -43,19 +45,22 @@ func TestMeterProviderRace(t *testing.T) {
 
 	mp.setDelegate(noop.NewMeterProvider())
 	close(finish)
+	<-done
 }
 
 var zeroCallback metric.Callback = func(ctx context.Context, or metric.Observer) error {
 	return nil
 }
 
-func TestMeterRace(t *testing.T) {
+func TestMeterConcurrentSafe(t *testing.T) {
 	mtr := &meter{}
 
 	wg := &sync.WaitGroup{}
 	wg.Add(1)
+	done := make(chan struct{})
 	finish := make(chan struct{})
 	go func() {
+		defer close(done)
 		for i, once := 0, false; ; i++ {
 			name := fmt.Sprintf("a%d", i)
 			_, _ = mtr.Float64ObservableCounter(name)
@@ -86,17 +91,20 @@ func TestMeterRace(t *testing.T) {
 	wg.Wait()
 	mtr.setDelegate(noop.NewMeterProvider())
 	close(finish)
+	<-done
 }
 
-func TestUnregisterRace(t *testing.T) {
+func TestUnregisterConcurrentSafe(t *testing.T) {
 	mtr := &meter{}
 	reg, err := mtr.RegisterCallback(zeroCallback)
 	require.NoError(t, err)
 
 	wg := &sync.WaitGroup{}
 	wg.Add(1)
+	done := make(chan struct{})
 	finish := make(chan struct{})
 	go func() {
+		defer close(done)
 		for i, once := 0, false; ; i++ {
 			_ = reg.Unregister()
 			if !once {
@@ -115,6 +123,7 @@ func TestUnregisterRace(t *testing.T) {
 	wg.Wait()
 	mtr.setDelegate(noop.NewMeterProvider())
 	close(finish)
+	<-done
 }
 
 func testSetupAllInstrumentTypes(t *testing.T, m metric.Meter) (metric.Float64Counter, metric.Float64ObservableCounter) {

metric/instrument_test.go

@@ -101,7 +101,7 @@ func testConfAttr(newConf func(...MeasurementOption) attrConf) func(t *testing.T
 	}
 }
 
-func TestWithAttributesRace(t *testing.T) {
+func TestWithAttributesConcurrentSafe(t *testing.T) {
 	attrs := []attribute.KeyValue{
 		attribute.String("user", "Alice"),
 		attribute.Bool("admin", true),

sdk/metric/cache_test.go

@@ -40,7 +40,7 @@ func TestCache(t *testing.T) {
 	assert.Equal(t, v1, c.Lookup(k1, func() int { return v1 }), "non-existing key")
 }
 
-func TestCacheConcurrency(t *testing.T) {
+func TestCacheConcurrentSafe(t *testing.T) {
 	const (
 		key        = "k"
 		goroutines = 10

sdk/metric/internal/aggregate/aggregator_test.go

@@ -78,7 +78,7 @@ func (at *aggregatorTester[N]) Run(a aggregator[N], incr setMap[N], eFunc expect
 			})
 		})
 
-		t.Run("Correctness", func(t *testing.T) {
+		t.Run("CorrectnessConcurrentSafe", func(t *testing.T) {
 			for i := 0; i < at.CycleN; i++ {
 				var wg sync.WaitGroup
 				wg.Add(at.GoroutineN)

sdk/metric/meter_test.go

@@ -37,7 +37,7 @@ import (
 )
 
 // A meter should be able to make instruments concurrently.
-func TestMeterInstrumentConcurrency(t *testing.T) {
+func TestMeterInstrumentConcurrentSafe(t *testing.T) {
 	wg := &sync.WaitGroup{}
 	wg.Add(12)
 

sdk/metric/pipeline_test.go

@@ -81,7 +81,7 @@ func TestPipelineUsesResource(t *testing.T) {
 	assert.Equal(t, res, output.Resource)
 }
 
-func TestPipelineConcurrency(t *testing.T) {
+func TestPipelineConcurrentSafe(t *testing.T) {
 	pipe := newPipeline(nil, nil, nil)
 	ctx := context.Background()
 	var output metricdata.ResourceMetrics

sdk/metric/reader_test.go

@@ -157,7 +157,7 @@ func (ts *readerTestSuite) TestSDKFailureBlocksExternalProducer() {
 	ts.Equal(metricdata.ResourceMetrics{}, m)
 }
 
-func (ts *readerTestSuite) TestMethodConcurrency() {
+func (ts *readerTestSuite) TestMethodConcurrentSafe() {
 	// Requires the race-detector (a default test option for the project).
 
 	// All reader methods should be concurrent-safe.

sdk/trace/simple_span_processor_test.go

@@ -120,7 +120,7 @@ func TestSimpleSpanProcessorShutdown(t *testing.T) {
 	}
 }
 
-func TestSimpleSpanProcessorShutdownOnEndConcurrency(t *testing.T) {
+func TestSimpleSpanProcessorShutdownOnEndConcurrentSafe(t *testing.T) {
 	exporter := &testExporter{}
 	ssp := sdktrace.NewSimpleSpanProcessor(exporter)
 	tp := basicTracerProvider(t)
@@ -153,7 +153,7 @@ func TestSimpleSpanProcessorShutdownOnEndConcurrency(t *testing.T) {
 	<-done
 }
 
-func TestSimpleSpanProcessorShutdownOnEndRace(t *testing.T) {
+func TestSimpleSpanProcessorShutdownOnEndConcurrentSafe2(t *testing.T) {
 	exporter := &testExporter{}
 	ssp := sdktrace.NewSimpleSpanProcessor(exporter)
 	tp := basicTracerProvider(t)