Merge pull request #13052 from ptabor/20210527-before-test-public

integration.BeforeTest can be run without leak-detection.

client/pkg/testutil/leak.go

@@ -16,6 +16,8 @@ import (
 	"time"
 )
 
+// TODO: Replace with https://github.com/uber-go/goleak.
+
 /*
 CheckLeakedGoroutine verifies tests do not leave any leaky
 goroutines. It returns true when there are goroutines still
@@ -28,10 +30,9 @@ running(leaking) after all tests.
 	}
 
 	func TestSample(t *testing.T) {
-		BeforeTest(t)
+		RegisterLeakDetection(t)
 		...
 	}
-
 */
 func CheckLeakedGoroutine() bool {
 	gs := interestingGoroutines()
@@ -94,22 +95,22 @@ func CheckAfterTest(d time.Duration) error {
 	return fmt.Errorf("appears to have leaked %s:\n%s", bad, stacks)
 }
 
-// BeforeTest is a convenient way to register before-and-after code to a test.
-// If you execute BeforeTest, you don't need to explicitly register AfterTest.
-func BeforeTest(t TB) {
+// RegisterLeakDetection is a convenient way to register before-and-after code to a test.
+// If you execute RegisterLeakDetection, you don't need to explicitly register AfterTest.
+func RegisterLeakDetection(t TB) {
 	if err := CheckAfterTest(10 * time.Millisecond); err != nil {
 		t.Skip("Found leaked goroutined BEFORE test", err)
 		return
 	}
 	t.Cleanup(func() {
-		AfterTest(t)
+		afterTest(t)
 	})
 }
 
-// AfterTest is meant to run in a defer that executes after a test completes.
+// afterTest is meant to run in a defer that executes after a test completes.
 // It will detect common goroutine leaks, retrying in case there are goroutines
 // not synchronously torn down, and fail the test if any goroutines are stuck.
-func AfterTest(t TB) {
+func afterTest(t TB) {
 	// If test-failed the leaked goroutines list is hidding the real
 	// source of problem.
 	if !t.Failed() {

client/pkg/testutil/leak_test.go

@@ -35,7 +35,7 @@ func TestMain(m *testing.M) {
 
 func TestSample(t *testing.T) {
 	SkipTestIfShortMode(t, "Counting leaked routines is disabled in --short tests")
-	defer AfterTest(t)
+	defer afterTest(t)
 	ranSample = true
 	for range make([]struct{}, 100) {
 		go func() {

client/v3/client_test.go

@@ -37,7 +37,7 @@ func NewClient(t *testing.T, cfg Config) (*Client, error) {
 }
 
 func TestDialCancel(t *testing.T) {
-	testutil.BeforeTest(t)
+	testutil.RegisterLeakDetection(t)
 
 	// accept first connection so client is created with dial timeout
 	ln, err := net.Listen("unix", "dialcancel:12345")
@@ -89,7 +89,7 @@ func TestDialCancel(t *testing.T) {
 }
 
 func TestDialTimeout(t *testing.T) {
-	testutil.BeforeTest(t)
+	testutil.RegisterLeakDetection(t)
 
 	wantError := context.DeadlineExceeded
 

client/v3/txn_test.go

@@ -23,7 +23,7 @@ import (
 )
 
 func TestTxnPanics(t *testing.T) {
-	testutil.BeforeTest(t)
+	testutil.RegisterLeakDetection(t)
 
 	kv := &kv{}
 

server/etcdserver/api/rafthttp/stream_test.go

@@ -187,7 +187,7 @@ func TestStreamReaderDialResult(t *testing.T) {
 
 // TestStreamReaderStopOnDial tests a stream reader closes the connection on stop.
 func TestStreamReaderStopOnDial(t *testing.T) {
-	testutil.BeforeTest(t)
+	testutil.RegisterLeakDetection(t)
 	h := http.Header{}
 	h.Add("X-Server-Version", version.Version)
 	tr := &respWaitRoundTripper{rrt: &respRoundTripper{code: http.StatusOK, header: h}}

tests/e2e/testing.go

@@ -25,7 +25,7 @@ import (
 
 func BeforeTest(t testing.TB) {
 	skipInShortMode(t)
-	testutil.BeforeTest(t)
+	testutil.RegisterLeakDetection(t)
 	os.Setenv(verify.ENV_VERIFY, verify.ENV_VERIFY_ALL_VALUE)
 
 	path, err := os.Getwd()

tests/integration/clientv3/connectivity/server_shutdown_test.go

@@ -17,6 +17,7 @@ package connectivity_test
 import (
 	"bytes"
 	"context"
+	"fmt"
 	"testing"
 	"time"
 
@@ -243,8 +244,10 @@ func TestBalancerUnderServerStopInflightLinearizableGetOnRestart(t *testing.T) {
 		{pinLeader: false, stopPinFirst: true},
 		{pinLeader: false, stopPinFirst: false},
 	}
-	for i := range tt {
-		testBalancerUnderServerStopInflightRangeOnRestart(t, true, tt[i])
+	for _, w := range tt {
+		t.Run(fmt.Sprintf("%#v", w), func(t *testing.T) {
+			testBalancerUnderServerStopInflightRangeOnRestart(t, true, w)
+		})
 	}
 }
 
@@ -255,8 +258,10 @@ func TestBalancerUnderServerStopInflightSerializableGetOnRestart(t *testing.T) {
 		{pinLeader: false, stopPinFirst: true},
 		{pinLeader: false, stopPinFirst: false},
 	}
-	for i := range tt {
-		testBalancerUnderServerStopInflightRangeOnRestart(t, false, tt[i])
+	for _, w := range tt {
+		t.Run(fmt.Sprintf("%#v", w), func(t *testing.T) {
+			testBalancerUnderServerStopInflightRangeOnRestart(t, false, w)
+		})
 	}
 }
 

tests/integration/clientv3/watch_test.go

@@ -611,8 +611,6 @@ func TestConfigurableWatchProgressNotifyInterval(t *testing.T) {
 }
 
 func TestWatchRequestProgress(t *testing.T) {
-	integration.BeforeTest(t)
-
 	if integration.ThroughProxy {
 		t.Skipf("grpc-proxy does not support WatchProgress yet")
 	}

tests/integration/cluster.go

@@ -1289,16 +1289,8 @@ type ClusterV3 struct {
 // for each cluster member.
 func NewClusterV3(t testutil.TB, cfg *ClusterConfig) *ClusterV3 {
 	t.Helper()
-	testutil.SkipTestIfShortMode(t, "Cannot create clusters in --short tests")
 
-	wd, err := os.Getwd()
-	if err != nil {
-		t.Fatal(err)
-	}
-	if !strings.HasPrefix(wd, os.TempDir()) {
-		t.Errorf("Working directory '%s' expected to be in temp-dir ('%s')."+
-			"Have you executed integration.BeforeTest(t) ?", wd, os.TempDir())
-	}
+	assertInTestContext(t)
 
 	cfg.UseGRPC = true
 

tests/integration/testing.go

@@ -30,29 +30,85 @@ import (
 )
 
 var grpc_logger grpc_logsettable.SettableLoggerV2
+var insideTestContext bool
 
 func init() {
 	grpc_logger = grpc_logsettable.ReplaceGrpcLoggerV2()
 }
 
-func BeforeTest(t testutil.TB) {
-	testutil.BeforeTest(t)
+type testOptions struct {
+	goLeakDetection bool
+	skipInShort     bool
+}
 
-	grpc_logger.Set(zapgrpc.NewLogger(zaptest.NewLogger(t).Named("grpc")))
+func newTestOptions(opts ...TestOption) *testOptions {
+	o := &testOptions{goLeakDetection: true, skipInShort: true}
+	for _, opt := range opts {
+		opt(o)
+	}
+	return o
+}
 
-	// Integration tests should verify written state as much as possible.
-	os.Setenv(verify.ENV_VERIFY, verify.ENV_VERIFY_ALL_VALUE)
+type TestOption func(opt *testOptions)
+
+// WithoutGoLeakDetection disables checking whether a testcase leaked a goroutine.
+func WithoutGoLeakDetection() TestOption {
+	return func(opt *testOptions) { opt.goLeakDetection = false }
+}
+
+func WithoutSkipInShort() TestOption {
+	return func(opt *testOptions) { opt.skipInShort = false }
+}
+
+// BeforeTestExternal initializes test context and is targeted for external APIs.
+// In general the `integration` package is not targeted to be used outside of
+// etcd project, but till the dedicated package is developed, this is
+// the best entry point so far (without backward compatibility promise).
+func BeforeTestExternal(t testutil.TB) {
+	BeforeTest(t, WithoutSkipInShort(), WithoutGoLeakDetection())
+}
+
+func BeforeTest(t testutil.TB, opts ...TestOption) {
+	t.Helper()
+	options := newTestOptions(opts...)
+
+	if options.skipInShort {
+		testutil.SkipTestIfShortMode(t, "Cannot create clusters in --short tests")
+	}
+
+	if options.goLeakDetection {
+		testutil.RegisterLeakDetection(t)
+	}
 
 	previousWD, err := os.Getwd()
 	if err != nil {
 		t.Fatal(err)
 	}
-	os.Chdir(t.TempDir())
+	previousInsideTestContext := insideTestContext
+
+	// Registering cleanup early, such it will get executed even if the helper fails.
 	t.Cleanup(func() {
 		grpc_logger.Reset()
+		insideTestContext = previousInsideTestContext
 		os.Chdir(previousWD)
 	})
 
+	if insideTestContext {
+		t.Fatal("already in test context. BeforeTest was likely already called")
+	}
+
+	grpc_logger.Set(zapgrpc.NewLogger(zaptest.NewLogger(t).Named("grpc")))
+	insideTestContext = true
+
+	// Integration tests should verify written state as much as possible.
+	os.Setenv(verify.ENV_VERIFY, verify.ENV_VERIFY_ALL_VALUE)
+	os.Chdir(t.TempDir())
+}
+
+func assertInTestContext(t testutil.TB) {
+	if !insideTestContext {
+		t.Errorf("the function can be called only in the test context. Was integration.BeforeTest() called ?")
+	}
 }
 
 func MustAbsPath(path string) string {

tests/integration/testing_test.go

@@ -0,0 +1,28 @@
+// Copyright 2021 The etcd Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package integration_test
+
+import (
+	"testing"
+	"time"
+
+	"go.etcd.io/etcd/tests/v3/integration"
+)
+
+func TestBeforeTestWithoutLeakDetection(t *testing.T) {
+	integration.BeforeTest(t, integration.WithoutGoLeakDetection(), integration.WithoutSkipInShort())
+	// Intentional leak that should get ignored
+	go time.Sleep(2 * time.Second)
+}

tests/integration/v2store/store_tag_not_v2v3_test.go

@@ -33,7 +33,6 @@ type v2TestStore struct {
 func (s *v2TestStore) Close() {}
 
 func newTestStore(t *testing.T, ns ...string) StoreCloser {
-	integration.BeforeTest(t)
 	if len(ns) == 0 {
 		t.Logf("new v2 store with no namespace")
 	}
@@ -42,6 +41,7 @@ func newTestStore(t *testing.T, ns ...string) StoreCloser {
 
 // Ensure that the store can recover from a previously saved state.
 func TestStoreRecover(t *testing.T) {
+	integration.BeforeTest(t)
 	s := newTestStore(t)
 	defer s.Close()
 	var eidx uint64 = 4

tests/integration/v3_lease_test.go

@@ -233,7 +233,6 @@ func TestV3LeaseCheckpoint(t *testing.T) {
 
 	var ttl int64 = 300
 	leaseInterval := 2 * time.Second
-	BeforeTest(t)
 	clus := NewClusterV3(t, &ClusterConfig{
 		Size:                    3,
 		EnableLeaseCheckpoint:   true,