Skip to content

Commit

Permalink
styleguide: tests goroutine leaks and naming (#4348)
Browse files Browse the repository at this point in the history
* internal/global: Fix goroutine leaks in tests
  • Loading branch information
pellared authored Jul 24, 2023
1 parent cbc5890 commit fd5284f
Show file tree
Hide file tree
Showing 16 changed files with 90 additions and 40 deletions.
7 changes: 7 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion exporters/otlp/internal/retry/retry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion exporters/otlp/otlpmetric/internal/exporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand Down
2 changes: 1 addition & 1 deletion exporters/otlp/otlptrace/otlptracehttp/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
23 changes: 19 additions & 4 deletions internal/global/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import (
"errors"
"io"
"log"
"os"
"sync"
"testing"

"github.com/stretchr/testify/suite"
Expand Down Expand Up @@ -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) {
Expand Down
38 changes: 22 additions & 16 deletions internal/global/instruments_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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)
})
})

Expand All @@ -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)
})
})

Expand All @@ -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)
})
})
}
Expand Down
21 changes: 17 additions & 4 deletions internal/global/internal_logging_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ package global
import (
"bytes"
"errors"
"io"
"log"
"os"
"sync"
"testing"

"github.com/go-logr/logr"
Expand All @@ -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) {
Expand Down
15 changes: 12 additions & 3 deletions internal/global/meter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion metric/instrument_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion sdk/metric/cache_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion sdk/metric/internal/aggregate/aggregator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion sdk/metric/meter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion sdk/metric/pipeline_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion sdk/metric/reader_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions sdk/trace/simple_span_processor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit fd5284f

Please sign in to comment.