Skip to content

Commit

Permalink
Feature: add goleak to check goroutine leak in tests (#5010)
Browse files Browse the repository at this point in the history
## Which problem is this PR solving?
- Partially Fixes #5006 

## Description of the changes
- added a linter to check implementation of goleak in tests
- added go leak in some tests 

## How was this change tested?
- 

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [ ] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Harshvir Potpose <hpotpose62@gmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>
  • Loading branch information
3 people authored Dec 18, 2023
1 parent 9f63383 commit 74293cd
Show file tree
Hide file tree
Showing 11 changed files with 75 additions and 2 deletions.
9 changes: 7 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ ALL_SRC = $(shell find . -name '*.go' \
-type f | \
sort)

# ALL_PKGS is used with 'nocover'
# ALL_PKGS is used with 'nocover' and 'goleak'
ALL_PKGS = $(shell echo $(dir $(ALL_SRC)) | tr ' ' '\n' | sort -u)

UNAME := $(shell uname -m)
Expand Down Expand Up @@ -162,6 +162,11 @@ nocover:
@echo Verifying that all packages have test files to count in coverage
@scripts/check-test-files.sh $(ALL_PKGS)

.PHONY: goleak
goleak:
@echo Verifying that all packages with tests have goleak in their TestMain
@scripts/check-goleak-files.sh $(ALL_PKGS)

.PHONY: fmt
fmt:
./scripts/import-order-cleanup.sh inplace
Expand All @@ -172,7 +177,7 @@ fmt:
./scripts/updateLicenses.sh

.PHONY: lint
lint:
lint: goleak
golangci-lint -v run
./scripts/updateLicenses.sh > $(FMT_LOG)
./scripts/import-order-cleanup.sh stdout > $(IMPORT_LOG)
Expand Down
5 changes: 5 additions & 0 deletions cmd/agent/app/configmanager/grpc/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/goleak"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"

Expand Down Expand Up @@ -81,3 +82,7 @@ func initializeGRPCTestServer(t *testing.T, beforeServe func(server *grpc.Server
}()
return server, lis.Addr()
}

func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}
5 changes: 5 additions & 0 deletions cmd/agent/app/customtransport/buffered_read_transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"testing"

"github.com/stretchr/testify/require"
"go.uber.org/goleak"
)

// TestTBufferedReadTransport tests the TBufferedReadTransport
Expand Down Expand Up @@ -69,3 +70,7 @@ func TestTBufferedReadTransportEmptyFunctions(t *testing.T) {
isOpen := trans.IsOpen()
require.True(t, isOpen)
}

func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}
5 changes: 5 additions & 0 deletions cmd/agent/app/httpserver/srv_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,15 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"go.uber.org/goleak"
"go.uber.org/zap"
)

func TestHTTPServer(t *testing.T) {
s := NewHTTPServer(":1", nil, nil, zap.NewNop())
assert.NotNil(t, s)
}

func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}
5 changes: 5 additions & 0 deletions cmd/agent/app/processors/thrift_processor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/apache/thrift/lib/go/thrift"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/goleak"
"go.uber.org/zap/zaptest"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
Expand Down Expand Up @@ -243,3 +244,7 @@ func assertCollectorReceivedData(
{Name: "thrift.udp.server.packets.processed", Value: 1},
}...)
}

func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}
5 changes: 5 additions & 0 deletions cmd/agent/app/servers/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"go.uber.org/goleak"
)

func TestReadBuf_EOF(t *testing.T) {
Expand All @@ -37,3 +38,7 @@ func TestReadBuf_Read(t *testing.T) {
assert.Equal(t, 5, n)
assert.Equal(t, "hello", string(r))
}

func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}
5 changes: 5 additions & 0 deletions cmd/agent/app/servers/thriftudp/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/goleak"
)

var localListenAddr = &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1)}
Expand Down Expand Up @@ -239,3 +240,7 @@ func TestCreateClient(t *testing.T) {
_, err := createClient(nil, nil)
assert.EqualError(t, err, "dial udp: missing address")
}

func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ require (
go.opentelemetry.io/otel/trace v1.21.0
go.uber.org/atomic v1.11.0
go.uber.org/automaxprocs v1.5.3
go.uber.org/goleak v1.3.0
go.uber.org/zap v1.26.0
golang.org/x/net v0.19.0
golang.org/x/sys v0.15.0
Expand Down
1 change: 1 addition & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -766,6 +766,7 @@ go.uber.org/automaxprocs v1.5.3 h1:kWazyxZUrS3Gs4qUpbwo5kEIMGe/DAvi5Z4tl2NW4j8=
go.uber.org/automaxprocs v1.5.3/go.mod h1:eRbA25aqJrxAbsLO0xy5jVwPt7FQnRgjW+efnwa1WM0=
go.uber.org/goleak v1.1.10/go.mod h1:8a7PlsEVH3e/a/GLqe5IIrQx6GzcnRmZEufDUTk4A7A=
go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
go.uber.org/multierr v1.11.0 h1:blXXJkSxSSfBVBlC76pxqeO+LN3aDfLQo+309xJstO0=
go.uber.org/multierr v1.11.0/go.mod h1:20+QtiLqy0Nd6FdQB9TLXag12DsQkrbs3htMFfDN80Y=
Expand Down
5 changes: 5 additions & 0 deletions model/trace_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"time"

"github.com/stretchr/testify/assert"
"go.uber.org/goleak"

"github.com/jaegertracing/jaeger/model"
)
Expand Down Expand Up @@ -66,3 +67,7 @@ func TestTraceNormalizeTimestamps(t *testing.T) {
assert.Equal(t, span.StartTime, tt1.UTC())
assert.Equal(t, span.Logs[0].Timestamp, tt2.UTC())
}

func TestMain(m *testing.M) {
goleak.VerifyTestMain(m)
}
31 changes: 31 additions & 0 deletions scripts/check-goleak-files.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#!/bin/bash

set -euo pipefail

bad_pkgs=0

# shellcheck disable=SC2048
for dir in $*; do
if [[ -f "${dir}/.nocover" ]]; then
continue
fi
testFiles=$(find "${dir}" -maxdepth 1 -name '*_test.go')
good=0
for test in ${testFiles}; do
if grep -q "TestMain" "${test}" | grep -q "goleak.VerifyTestMain" "${test}"; then
good=1
break
fi
done
if ((good == 0)); then
echo "🔴 Error(check-goleak): no goleak check in package ${dir}"
((bad_pkgs+=1))
fi
done

if ((bad_pkgs > 0)); then
echo "Error(check-goleak): no goleak check in ${bad_pkgs} package(s)."
echo "See https://github.com/jaegertracing/jaeger/pull/5010/files for example of adding the checks."
echo "In the future this will be a fatal error in the CI."
exit 0 # TODO change to 1 in the future
fi

0 comments on commit 74293cd

Please sign in to comment.