From 3f13d3a2d5c1b5832b6551776aa0f8ead572494c Mon Sep 17 00:00:00 2001 From: Piotr Tabor Date: Thu, 27 May 2021 18:44:14 +0200 Subject: [PATCH] integration.BeforeTest can be run without leak-detection. --- client/pkg/testutil/leak.go | 17 ++--- client/pkg/testutil/leak_test.go | 2 +- client/v3/client_test.go | 4 +- client/v3/txn_test.go | 2 +- server/etcdserver/api/rafthttp/stream_test.go | 2 +- tests/e2e/testing.go | 2 +- .../connectivity/server_shutdown_test.go | 13 ++-- tests/integration/clientv3/watch_test.go | 2 - tests/integration/cluster.go | 10 +-- tests/integration/testing.go | 68 +++++++++++++++++-- tests/integration/testing_test.go | 28 ++++++++ .../v2store/store_tag_not_v2v3_test.go | 2 +- tests/integration/v3_lease_test.go | 1 - 13 files changed, 116 insertions(+), 37 deletions(-) create mode 100644 tests/integration/testing_test.go diff --git a/client/pkg/testutil/leak.go b/client/pkg/testutil/leak.go index f786b5ccd..e9ffbd0ff 100644 --- a/client/pkg/testutil/leak.go +++ b/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() { diff --git a/client/pkg/testutil/leak_test.go b/client/pkg/testutil/leak_test.go index fa70782f3..71b1c7bf3 100644 --- a/client/pkg/testutil/leak_test.go +++ b/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() { diff --git a/client/v3/client_test.go b/client/v3/client_test.go index b2ff7ef17..0804f05c5 100644 --- a/client/v3/client_test.go +++ b/client/v3/client_test.go @@ -35,7 +35,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") @@ -87,7 +87,7 @@ func TestDialCancel(t *testing.T) { } func TestDialTimeout(t *testing.T) { - testutil.BeforeTest(t) + testutil.RegisterLeakDetection(t) wantError := context.DeadlineExceeded diff --git a/client/v3/txn_test.go b/client/v3/txn_test.go index 4be010013..012c965a5 100644 --- a/client/v3/txn_test.go +++ b/client/v3/txn_test.go @@ -23,7 +23,7 @@ import ( ) func TestTxnPanics(t *testing.T) { - testutil.BeforeTest(t) + testutil.RegisterLeakDetection(t) kv := &kv{} diff --git a/server/etcdserver/api/rafthttp/stream_test.go b/server/etcdserver/api/rafthttp/stream_test.go index 479505e4a..5006bd089 100644 --- a/server/etcdserver/api/rafthttp/stream_test.go +++ b/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}} diff --git a/tests/e2e/testing.go b/tests/e2e/testing.go index a36c075db..00e98114e 100644 --- a/tests/e2e/testing.go +++ b/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() diff --git a/tests/integration/clientv3/connectivity/server_shutdown_test.go b/tests/integration/clientv3/connectivity/server_shutdown_test.go index fc5f18fb2..8ab90cbc5 100644 --- a/tests/integration/clientv3/connectivity/server_shutdown_test.go +++ b/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) + }) } } diff --git a/tests/integration/clientv3/watch_test.go b/tests/integration/clientv3/watch_test.go index aab6b6b13..2fea3c9ba 100644 --- a/tests/integration/clientv3/watch_test.go +++ b/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") } diff --git a/tests/integration/cluster.go b/tests/integration/cluster.go index c5302e429..b7e36817b 100644 --- a/tests/integration/cluster.go +++ b/tests/integration/cluster.go @@ -1288,16 +1288,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 diff --git a/tests/integration/testing.go b/tests/integration/testing.go index 8d0ee40ff..e67375180 100644 --- a/tests/integration/testing.go +++ b/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 { diff --git a/tests/integration/testing_test.go b/tests/integration/testing_test.go new file mode 100644 index 000000000..f9afb8ded --- /dev/null +++ b/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) +} diff --git a/tests/integration/v2store/store_tag_not_v2v3_test.go b/tests/integration/v2store/store_tag_not_v2v3_test.go index fbc63b94a..1b31ce6c8 100644 --- a/tests/integration/v2store/store_tag_not_v2v3_test.go +++ b/tests/integration/v2store/store_tag_not_v2v3_test.go @@ -32,7 +32,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") } @@ -41,6 +40,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 diff --git a/tests/integration/v3_lease_test.go b/tests/integration/v3_lease_test.go index 41f33ef62..08b0ca7bb 100644 --- a/tests/integration/v3_lease_test.go +++ b/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,