From fd5284f75cfc69d07a61024793e0ed09a8dc9c23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20Paj=C4=85k?= Date: Mon, 24 Jul 2023 09:35:06 +0200 Subject: [PATCH] styleguide: tests goroutine leaks and naming (#4348) * internal/global: Fix goroutine leaks in tests --- CONTRIBUTING.md | 7 ++++ exporters/otlp/internal/retry/retry_test.go | 2 +- .../otlp/otlpmetric/internal/exporter_test.go | 2 +- .../otlptrace/otlptracehttp/client_test.go | 2 +- internal/global/handler_test.go | 23 +++++++++-- internal/global/instruments_test.go | 38 +++++++++++-------- internal/global/internal_logging_test.go | 21 ++++++++-- internal/global/meter_test.go | 15 ++++++-- metric/instrument_test.go | 2 +- sdk/metric/cache_test.go | 2 +- .../internal/aggregate/aggregator_test.go | 2 +- sdk/metric/meter_test.go | 2 +- sdk/metric/pipeline_test.go | 2 +- sdk/metric/reader_test.go | 2 +- sdk/trace/simple_span_processor_test.go | 4 +- sdk/trace/tracetest/recorder_test.go | 4 +- 16 files changed, 90 insertions(+), 40 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 72c1b9eb13c..03bcad6db20 100644 --- a/CONTRIBUTING.md +++ b/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 diff --git a/exporters/otlp/internal/retry/retry_test.go b/exporters/otlp/internal/retry/retry_test.go index ad61bb306d0..de574a73579 100644 --- a/exporters/otlp/internal/retry/retry_test.go +++ b/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, diff --git a/exporters/otlp/otlpmetric/internal/exporter_test.go b/exporters/otlp/otlpmetric/internal/exporter_test.go index a9f250fbd7c..9f071032f7b 100644 --- a/exporters/otlp/otlpmetric/internal/exporter_test.go +++ b/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{}) diff --git a/exporters/otlp/otlptrace/otlptracehttp/client_test.go b/exporters/otlp/otlptrace/otlptracehttp/client_test.go index 859c48dd38c..9c6a150d28a 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/client_test.go +++ b/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) diff --git a/internal/global/handler_test.go b/internal/global/handler_test.go index 08074bcc0ff..6ddae2f4cce 100644 --- a/internal/global/handler_test.go +++ b/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) { diff --git a/internal/global/instruments_test.go b/internal/global/instruments_test.go index 7003e04b1f2..c0ab914b6c2 100644 --- a/internal/global/instruments_test.go +++ 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) }) }) } diff --git a/internal/global/internal_logging_test.go b/internal/global/internal_logging_test.go index ae2f2f61781..b51333b4dd5 100644 --- a/internal/global/internal_logging_test.go +++ b/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) { diff --git a/internal/global/meter_test.go b/internal/global/meter_test.go index b91f4fd57c8..9ad8d4f5ee7 100644 --- a/internal/global/meter_test.go +++ b/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) { diff --git a/metric/instrument_test.go b/metric/instrument_test.go index 4574160839c..0d2d1b067fc 100644 --- a/metric/instrument_test.go +++ b/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), diff --git a/sdk/metric/cache_test.go b/sdk/metric/cache_test.go index 47332a58cbd..d70b3b9e2cb 100644 --- a/sdk/metric/cache_test.go +++ b/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 diff --git a/sdk/metric/internal/aggregate/aggregator_test.go b/sdk/metric/internal/aggregate/aggregator_test.go index 80882c7d641..0bca6b01b4b 100644 --- a/sdk/metric/internal/aggregate/aggregator_test.go +++ b/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) diff --git a/sdk/metric/meter_test.go b/sdk/metric/meter_test.go index 8657ccc7135..bfa1879120b 100644 --- a/sdk/metric/meter_test.go +++ b/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) diff --git a/sdk/metric/pipeline_test.go b/sdk/metric/pipeline_test.go index 58916079429..fdeb4fe1f7b 100644 --- a/sdk/metric/pipeline_test.go +++ b/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 diff --git a/sdk/metric/reader_test.go b/sdk/metric/reader_test.go index ca10da77340..d375a1b4f6c 100644 --- a/sdk/metric/reader_test.go +++ b/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. diff --git a/sdk/trace/simple_span_processor_test.go b/sdk/trace/simple_span_processor_test.go index fc038978314..38a44e08fb2 100644 --- a/sdk/trace/simple_span_processor_test.go +++ b/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) diff --git a/sdk/trace/tracetest/recorder_test.go b/sdk/trace/tracetest/recorder_test.go index ef292a981ce..de7eead160c 100644 --- a/sdk/trace/tracetest/recorder_test.go +++ b/sdk/trace/tracetest/recorder_test.go @@ -99,7 +99,7 @@ func runConcurrently(funcs ...func()) { wg.Wait() } -func TestEndingConcurrency(t *testing.T) { +func TestEndingConcurrentSafe(t *testing.T) { sr := NewSpanRecorder() runConcurrently( @@ -111,7 +111,7 @@ func TestEndingConcurrency(t *testing.T) { assert.Len(t, sr.Ended(), 2) } -func TestStartingConcurrency(t *testing.T) { +func TestStartingConcurrentSafe(t *testing.T) { sr := NewSpanRecorder() ctx := context.Background()