From f473cf7d4afd5bd1ff3a6824d9367d0ea8c8b2fa Mon Sep 17 00:00:00 2001 From: Tenghu Zhang Date: Sat, 31 Jul 2021 09:29:27 +0000 Subject: [PATCH 1/6] migrate test-infra to testify for owner pkg Signed-off-by: hu00yan --- owner/fail_test.go | 44 +++++++++++++++---------------------------- owner/main_test.go | 26 +++++++++++++++++++++++++ owner/manager_test.go | 14 +++++--------- 3 files changed, 46 insertions(+), 38 deletions(-) create mode 100644 owner/main_test.go diff --git a/owner/fail_test.go b/owner/fail_test.go index 3ccc8c7632fc4..42cf1243ac137 100644 --- a/owner/fail_test.go +++ b/owner/fail_test.go @@ -24,11 +24,11 @@ import ( "testing" "time" - . "github.com/pingcap/check" "github.com/pingcap/failpoint" "github.com/pingcap/parser/terror" + "github.com/pingcap/tidb/util/logutil" - "github.com/pingcap/tidb/util/testleak" + "github.com/stretchr/testify/require" "go.etcd.io/etcd/clientv3" "google.golang.org/grpc" ) @@ -36,24 +36,11 @@ import ( // Ignore this test on the windows platform, because calling unix socket with address in // host:port format fails on windows. func TestT(t *testing.T) { - CustomVerboseFlag = true logLevel := os.Getenv("log_level") err := logutil.InitLogger(logutil.NewLogConfig(logLevel, "", "", logutil.EmptyFileLogConfig, false)) if err != nil { t.Fatal(err) } - TestingT(t) -} - -var _ = Suite(&testSuite{}) - -type testSuite struct { -} - -func (s *testSuite) SetUpSuite(c *C) { -} - -func (s *testSuite) TearDownSuite(c *C) { } var ( @@ -61,28 +48,27 @@ var ( retryCnt = math.MaxInt32 ) -func (s *testSuite) TestFailNewSession(c *C) { +func TestFailNewSession(t *testing.T) { + t.Parallel() os.Remove("new_session:0") ln, err := net.Listen("unix", "new_session:0") - c.Assert(err, IsNil) + require.NoError(t, err) addr := ln.Addr() endpoints := []string{fmt.Sprintf("%s://%s", addr.Network(), addr.String())} - c.Assert(err, IsNil) + require.Nil(t, err) srv := grpc.NewServer(grpc.ConnectionTimeout(time.Minute)) var stop sync.WaitGroup stop.Add(1) go func() { if err = srv.Serve(ln); err != nil { - c.Errorf("can't serve gRPC requests %v", err) + require.Errorf(t, err, "can't serve gRPC requests %v") } stop.Done() }() - leakFunc := testleak.AfterTest(c) defer func() { srv.Stop() stop.Wait() - leakFunc() }() func() { @@ -90,17 +76,17 @@ func (s *testSuite) TestFailNewSession(c *C) { Endpoints: endpoints, DialTimeout: dialTimeout, }) - c.Assert(err, IsNil) + require.NoError(t, err) defer func() { if cli != nil { cli.Close() } - c.Assert(failpoint.Disable("github.com/pingcap/tidb/owner/closeClient"), IsNil) + require.Nil(t, failpoint.Disable("github.com/pingcap/tidb/owner/closeClient")) }() - c.Assert(failpoint.Enable("github.com/pingcap/tidb/owner/closeClient", `return(true)`), IsNil) + require.NoError(t, err, failpoint.Enable("github.com/pingcap/tidb/owner/closeClient", `return(true)`)) _, err = NewSession(context.Background(), "fail_new_serssion", cli, retryCnt, ManagerSessionTTL) isContextDone := terror.ErrorEqual(grpc.ErrClientConnClosing, err) || terror.ErrorEqual(context.Canceled, err) - c.Assert(isContextDone, IsTrue, Commentf("err %v", err)) + require.True(t, isContextDone, "err %v", err) }() func() { @@ -108,17 +94,17 @@ func (s *testSuite) TestFailNewSession(c *C) { Endpoints: endpoints, DialTimeout: dialTimeout, }) - c.Assert(err, IsNil) + require.NoError(t, err) defer func() { if cli != nil { cli.Close() } - c.Assert(failpoint.Disable("github.com/pingcap/tidb/owner/closeGrpc"), IsNil) + require.Nil(t, failpoint.Disable("github.com/pingcap/tidb/owner/closeGrpc")) }() - c.Assert(failpoint.Enable("github.com/pingcap/tidb/owner/closeGrpc", `return(true)`), IsNil) + require.Nil(t, failpoint.Enable("github.com/pingcap/tidb/owner/closeGrpc", `return(false)`)) _, err = NewSession(context.Background(), "fail_new_serssion", cli, retryCnt, ManagerSessionTTL) isContextDone := terror.ErrorEqual(grpc.ErrClientConnClosing, err) || terror.ErrorEqual(context.Canceled, err) - c.Assert(isContextDone, IsTrue, Commentf("err %v", err)) + require.True(t, isContextDone, "err %v", err) }() } diff --git a/owner/main_test.go b/owner/main_test.go new file mode 100644 index 0000000000000..cd37d201a8bc2 --- /dev/null +++ b/owner/main_test.go @@ -0,0 +1,26 @@ +//Copyright 2021 PingCAP, Inc. +// +// 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, +// See the License for the specific language governing permissions and +// limitations under the License. + +package owner + +import ( + "testing" + + "github.com/pingcap/tidb/util/testbridge" + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + testbridge.WorkaroundGoCheckFlags() + goleak.VerifyTestMain(m) +} diff --git a/owner/manager_test.go b/owner/manager_test.go index 17536cd16610a..7ee6791357eaa 100644 --- a/owner/manager_test.go +++ b/owner/manager_test.go @@ -20,7 +20,6 @@ import ( "testing" "time" - . "github.com/pingcap/check" "github.com/pingcap/errors" "github.com/pingcap/parser/terror" . "github.com/pingcap/tidb/ddl" @@ -36,10 +35,6 @@ import ( const testLease = 5 * time.Millisecond -func TestT(t *testing.T) { - TestingT(t) -} - func checkOwner(d DDL, fbVal bool) (isOwner bool) { manager := d.OwnerManager() // The longest to wait for 30 seconds to @@ -123,6 +118,7 @@ func TestSingle(t *testing.T) { } func TestCluster(t *testing.T) { + t.Parallel() if runtime.GOOS == "windows" { t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") } @@ -194,7 +190,7 @@ func TestCluster(t *testing.T) { } err = d.Stop() if err != nil { - t.Fatal(err, IsNil) + t.Fatalf("DDL stop failed %v", err) } // d3 (not owner) stop @@ -215,7 +211,7 @@ func TestCluster(t *testing.T) { defer func() { err = d3.Stop() if err != nil { - t.Fatal(err, IsNil) + t.Fatalf("DDL stop failed %v", err) } }() isOwner = checkOwner(d3, false) @@ -224,13 +220,13 @@ func TestCluster(t *testing.T) { } err = d3.Stop() if err != nil { - t.Fatal(err, IsNil) + t.Fatalf("DDL stop failed %v", err) } // Cancel the owner context, there is no owner. err = d1.Stop() if err != nil { - t.Fatal(err, IsNil) + t.Fatalf("DDL stop failed %v", err) } time.Sleep(time.Duration(tmpTTL+1) * time.Second) session, err := concurrency.NewSession(cliRW) From 8fc4271a8fbd50e5d51baa2b6e5f8ef2d9b80e13 Mon Sep 17 00:00:00 2001 From: Tenghu Zhang Date: Sat, 31 Jul 2021 09:29:27 +0000 Subject: [PATCH 2/6] migrate test-infra to testify for owner pkg Signed-off-by: hu00yan --- owner/fail_test.go | 44 ++++++++++++---------------------- owner/main_test.go | 26 ++++++++++++++++++++ owner/manager_test.go | 55 ++++++++++++++++++++----------------------- 3 files changed, 67 insertions(+), 58 deletions(-) create mode 100644 owner/main_test.go diff --git a/owner/fail_test.go b/owner/fail_test.go index 3ccc8c7632fc4..42cf1243ac137 100644 --- a/owner/fail_test.go +++ b/owner/fail_test.go @@ -24,11 +24,11 @@ import ( "testing" "time" - . "github.com/pingcap/check" "github.com/pingcap/failpoint" "github.com/pingcap/parser/terror" + "github.com/pingcap/tidb/util/logutil" - "github.com/pingcap/tidb/util/testleak" + "github.com/stretchr/testify/require" "go.etcd.io/etcd/clientv3" "google.golang.org/grpc" ) @@ -36,24 +36,11 @@ import ( // Ignore this test on the windows platform, because calling unix socket with address in // host:port format fails on windows. func TestT(t *testing.T) { - CustomVerboseFlag = true logLevel := os.Getenv("log_level") err := logutil.InitLogger(logutil.NewLogConfig(logLevel, "", "", logutil.EmptyFileLogConfig, false)) if err != nil { t.Fatal(err) } - TestingT(t) -} - -var _ = Suite(&testSuite{}) - -type testSuite struct { -} - -func (s *testSuite) SetUpSuite(c *C) { -} - -func (s *testSuite) TearDownSuite(c *C) { } var ( @@ -61,28 +48,27 @@ var ( retryCnt = math.MaxInt32 ) -func (s *testSuite) TestFailNewSession(c *C) { +func TestFailNewSession(t *testing.T) { + t.Parallel() os.Remove("new_session:0") ln, err := net.Listen("unix", "new_session:0") - c.Assert(err, IsNil) + require.NoError(t, err) addr := ln.Addr() endpoints := []string{fmt.Sprintf("%s://%s", addr.Network(), addr.String())} - c.Assert(err, IsNil) + require.Nil(t, err) srv := grpc.NewServer(grpc.ConnectionTimeout(time.Minute)) var stop sync.WaitGroup stop.Add(1) go func() { if err = srv.Serve(ln); err != nil { - c.Errorf("can't serve gRPC requests %v", err) + require.Errorf(t, err, "can't serve gRPC requests %v") } stop.Done() }() - leakFunc := testleak.AfterTest(c) defer func() { srv.Stop() stop.Wait() - leakFunc() }() func() { @@ -90,17 +76,17 @@ func (s *testSuite) TestFailNewSession(c *C) { Endpoints: endpoints, DialTimeout: dialTimeout, }) - c.Assert(err, IsNil) + require.NoError(t, err) defer func() { if cli != nil { cli.Close() } - c.Assert(failpoint.Disable("github.com/pingcap/tidb/owner/closeClient"), IsNil) + require.Nil(t, failpoint.Disable("github.com/pingcap/tidb/owner/closeClient")) }() - c.Assert(failpoint.Enable("github.com/pingcap/tidb/owner/closeClient", `return(true)`), IsNil) + require.NoError(t, err, failpoint.Enable("github.com/pingcap/tidb/owner/closeClient", `return(true)`)) _, err = NewSession(context.Background(), "fail_new_serssion", cli, retryCnt, ManagerSessionTTL) isContextDone := terror.ErrorEqual(grpc.ErrClientConnClosing, err) || terror.ErrorEqual(context.Canceled, err) - c.Assert(isContextDone, IsTrue, Commentf("err %v", err)) + require.True(t, isContextDone, "err %v", err) }() func() { @@ -108,17 +94,17 @@ func (s *testSuite) TestFailNewSession(c *C) { Endpoints: endpoints, DialTimeout: dialTimeout, }) - c.Assert(err, IsNil) + require.NoError(t, err) defer func() { if cli != nil { cli.Close() } - c.Assert(failpoint.Disable("github.com/pingcap/tidb/owner/closeGrpc"), IsNil) + require.Nil(t, failpoint.Disable("github.com/pingcap/tidb/owner/closeGrpc")) }() - c.Assert(failpoint.Enable("github.com/pingcap/tidb/owner/closeGrpc", `return(true)`), IsNil) + require.Nil(t, failpoint.Enable("github.com/pingcap/tidb/owner/closeGrpc", `return(false)`)) _, err = NewSession(context.Background(), "fail_new_serssion", cli, retryCnt, ManagerSessionTTL) isContextDone := terror.ErrorEqual(grpc.ErrClientConnClosing, err) || terror.ErrorEqual(context.Canceled, err) - c.Assert(isContextDone, IsTrue, Commentf("err %v", err)) + require.True(t, isContextDone, "err %v", err) }() } diff --git a/owner/main_test.go b/owner/main_test.go new file mode 100644 index 0000000000000..cd37d201a8bc2 --- /dev/null +++ b/owner/main_test.go @@ -0,0 +1,26 @@ +//Copyright 2021 PingCAP, Inc. +// +// 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, +// See the License for the specific language governing permissions and +// limitations under the License. + +package owner + +import ( + "testing" + + "github.com/pingcap/tidb/util/testbridge" + "go.uber.org/goleak" +) + +func TestMain(m *testing.M) { + testbridge.WorkaroundGoCheckFlags() + goleak.VerifyTestMain(m) +} diff --git a/owner/manager_test.go b/owner/manager_test.go index 17536cd16610a..bab4f96865913 100644 --- a/owner/manager_test.go +++ b/owner/manager_test.go @@ -20,7 +20,6 @@ import ( "testing" "time" - . "github.com/pingcap/check" "github.com/pingcap/errors" "github.com/pingcap/parser/terror" . "github.com/pingcap/tidb/ddl" @@ -28,6 +27,7 @@ import ( "github.com/pingcap/tidb/owner" "github.com/pingcap/tidb/store/mockstore" "github.com/pingcap/tidb/util/logutil" + "github.com/stretchr/testify/require" "go.etcd.io/etcd/clientv3" "go.etcd.io/etcd/clientv3/concurrency" "go.etcd.io/etcd/integration" @@ -36,10 +36,6 @@ import ( const testLease = 5 * time.Millisecond -func TestT(t *testing.T) { - TestingT(t) -} - func checkOwner(d DDL, fbVal bool) (isOwner bool) { manager := d.OwnerManager() // The longest to wait for 30 seconds to @@ -60,12 +56,12 @@ func TestSingle(t *testing.T) { } store, err := mockstore.NewMockStore() if err != nil { - t.Fatal(err) + require.NoError(t, err) } defer func() { err := store.Close() if err != nil { - t.Fatal(err) + require.NoError(t, err) } }() @@ -84,7 +80,7 @@ func TestSingle(t *testing.T) { ) err = d.Start(nil) if err != nil { - t.Fatalf("DDL start failed %v", err) + require.NoErrorf(t, err, "DDL start failed %v") } defer func() { _ = d.Stop() @@ -92,7 +88,7 @@ func TestSingle(t *testing.T) { isOwner := checkOwner(d, true) if !isOwner { - t.Fatalf("expect true, got isOwner:%v", isOwner) + require.Emptyf(t, isOwner, "expet true, got isOowner:%v") } // test for newSession failed @@ -102,27 +98,28 @@ func TestSingle(t *testing.T) { err = manager.CampaignOwner() if !terror.ErrorEqual(err, goctx.Canceled) && !terror.ErrorEqual(err, goctx.DeadlineExceeded) { - t.Fatalf("campaigned result don't match, err %v", err) + require.NoErrorf(t, err, "campaigned result don't match, err %v") } isOwner = checkOwner(d, true) if !isOwner { - t.Fatalf("expect true, got isOwner:%v", isOwner) + require.NotEmptyf(t, isOwner, "expected true, got isOwner:%v") } // The test is used to exit campaign loop. d.OwnerManager().Cancel() isOwner = checkOwner(d, false) if isOwner { - t.Fatalf("expect false, got isOwner:%v", isOwner) + require.Emptyf(t, isOwner, "expected false, got isOwner:%v") } time.Sleep(200 * time.Millisecond) ownerID, _ := manager.GetOwnerID(goctx.Background()) // The error is ok to be not nil since we canceled the manager. if ownerID != "" { - t.Fatalf("owner %s is not empty", ownerID) + require.Emptyf(t, ownerID, "owner %s is not empty") } } func TestCluster(t *testing.T) { + t.Parallel() if runtime.GOOS == "windows" { t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") } @@ -134,12 +131,12 @@ func TestCluster(t *testing.T) { }() store, err := mockstore.NewMockStore() if err != nil { - t.Fatal(err) + require.NoError(t, err) } defer func() { err := store.Close() if err != nil { - t.Fatal(err) + require.NoError(t, err) } }() clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 4}) @@ -157,11 +154,11 @@ func TestCluster(t *testing.T) { ) err = d.Start(nil) if err != nil { - t.Fatalf("DDL start failed %v", err) + require.NoErrorf(t, err, "DDL start failed %v") } isOwner := checkOwner(d, true) if !isOwner { - t.Fatalf("expect true, got isOwner:%v", isOwner) + require.Emptyf(t, isOwner, "expect true, got isOwner:%v") } cli1 := clus.Client(1) ic2 := infoschema.NewCache(2) @@ -175,26 +172,26 @@ func TestCluster(t *testing.T) { ) err = d1.Start(nil) if err != nil { - t.Fatalf("DDL start failed %v", err) + require.NoErrorf(t, err, "DDL start failed %v") } isOwner = checkOwner(d1, false) if isOwner { - t.Fatalf("expect false, got isOwner:%v", isOwner) + require.Emptyf(t, isOwner, "expect false, got isOwner:%v") } // Delete the leader key, the d1 become the owner. cliRW := clus.Client(2) err = deleteLeader(cliRW, DDLOwnerKey) if err != nil { - t.Fatal(err) + require.NoError(t, err) } isOwner = checkOwner(d, false) if isOwner { - t.Fatalf("expect false, got isOwner:%v", isOwner) + require.Emptyf(t, isOwner, "expect false, got isOwner:%v") } err = d.Stop() if err != nil { - t.Fatal(err, IsNil) + require.NoErrorf(t, err, "DDL stop failed %v") } // d3 (not owner) stop @@ -210,39 +207,39 @@ func TestCluster(t *testing.T) { ) err = d3.Start(nil) if err != nil { - t.Fatalf("DDL start failed %v", err) + require.NoErrorf(t, err, "DDL start failed %v") } defer func() { err = d3.Stop() if err != nil { - t.Fatal(err, IsNil) + require.NoErrorf(t, err, "DDL stop failed %v") } }() isOwner = checkOwner(d3, false) if isOwner { - t.Fatalf("expect false, got isOwner:%v", isOwner) + require.Emptyf(t, isOwner, "expect false, got isOwner:%v") } err = d3.Stop() if err != nil { - t.Fatal(err, IsNil) + require.NoErrorf(t, err, "DDL stop failed %v") } // Cancel the owner context, there is no owner. err = d1.Stop() if err != nil { - t.Fatal(err, IsNil) + require.NoErrorf(t, err, "DDL stop failed %v") } time.Sleep(time.Duration(tmpTTL+1) * time.Second) session, err := concurrency.NewSession(cliRW) if err != nil { - t.Fatalf("new session failed %v", err) + require.NoErrorf(t, err, "new session failed %v") } elec := concurrency.NewElection(session, DDLOwnerKey) logPrefix := fmt.Sprintf("[ddl] %s ownerManager %s", DDLOwnerKey, "useless id") logCtx := logutil.WithKeyValue(context.Background(), "owner info", logPrefix) _, err = owner.GetOwnerInfo(goctx.Background(), logCtx, elec, "useless id") if !terror.ErrorEqual(err, concurrency.ErrElectionNoLeader) { - t.Fatalf("get owner info result don't match, err %v", err) + require.NoErrorf(t, err, "get owner info result don't match, err %v") } } From 7498babfce0ec8aaad5d61739d18b5f94d89880a Mon Sep 17 00:00:00 2001 From: Tenghu Zhang Date: Tue, 3 Aug 2021 02:08:40 +0000 Subject: [PATCH 3/6] fix unused and wrong Errorf --- owner/fail_test.go | 16 +------- owner/main_test.go | 13 ++++++ owner/manager_test.go | 92 +++++++++++-------------------------------- 3 files changed, 38 insertions(+), 83 deletions(-) diff --git a/owner/fail_test.go b/owner/fail_test.go index 9de95aaa29757..5ece39047cd06 100644 --- a/owner/fail_test.go +++ b/owner/fail_test.go @@ -27,22 +27,11 @@ import ( "github.com/pingcap/failpoint" "github.com/pingcap/parser/terror" - "github.com/pingcap/tidb/util/logutil" "github.com/stretchr/testify/require" "go.etcd.io/etcd/clientv3" "google.golang.org/grpc" ) -// Ignore this test on the windows platform, because calling unix socket with address in -// host:port format fails on windows. -func TestT(t *testing.T) { - logLevel := os.Getenv("log_level") - err := logutil.InitLogger(logutil.NewLogConfig(logLevel, "", "", logutil.EmptyFileLogConfig, false)) - if err != nil { - t.Fatal(err) - } -} - var ( dialTimeout = 5 * time.Second retryCnt = math.MaxInt32 @@ -60,9 +49,8 @@ func TestFailNewSession(t *testing.T) { var stop sync.WaitGroup stop.Add(1) go func() { - if err = srv.Serve(ln); err != nil { - require.Errorf(t, err, "can't serve gRPC requests %v") - } + err = srv.Serve(ln) + require.Errorf(t, err, "can't serve gRPC requests") stop.Done() }() diff --git a/owner/main_test.go b/owner/main_test.go index cd37d201a8bc2..33009f392184c 100644 --- a/owner/main_test.go +++ b/owner/main_test.go @@ -14,13 +14,26 @@ package owner import ( + "os" "testing" + "github.com/pingcap/log" + "github.com/pingcap/tidb/util/logutil" "github.com/pingcap/tidb/util/testbridge" "go.uber.org/goleak" + "go.uber.org/zap" ) func TestMain(m *testing.M) { testbridge.WorkaroundGoCheckFlags() + + // Ignore this test on the windows platform, because calling unix socket with address in + // host:port format fails on windows. + logLevel := os.Getenv("log_level") + err := logutil.InitLogger(logutil.NewLogConfig(logLevel, "", "", logutil.EmptyFileLogConfig, false)) + if err != nil { + log.Fatal("put failed", zap.Error(err)) + } + goleak.VerifyTestMain(m) } diff --git a/owner/manager_test.go b/owner/manager_test.go index 59810c9e966fd..af746283e4317 100644 --- a/owner/manager_test.go +++ b/owner/manager_test.go @@ -55,14 +55,10 @@ func TestSingle(t *testing.T) { t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") } store, err := mockstore.NewMockStore() - if err != nil { - require.NoError(t, err) - } + require.NoError(t, err) defer func() { err := store.Close() - if err != nil { - require.NoError(t, err) - } + require.NoError(t, err) }() clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 1}) @@ -79,17 +75,13 @@ func TestSingle(t *testing.T) { WithInfoCache(ic), ) err = d.Start(nil) - if err != nil { - require.NoErrorf(t, err, "DDL start failed %v") - } + require.NoErrorf(t, err, "DDL start failed") defer func() { _ = d.Stop() }() isOwner := checkOwner(d, true) - if !isOwner { - require.Emptyf(t, isOwner, "expet true, got isOowner:%v") - } + require.Truef(t, isOwner, "expet true, got isOowner false") // test for newSession failed ctx, cancel := goctx.WithCancel(ctx) @@ -101,21 +93,15 @@ func TestSingle(t *testing.T) { require.NoErrorf(t, err, "campaigned result don't match, err %v") } isOwner = checkOwner(d, true) - if !isOwner { - require.NotEmptyf(t, isOwner, "expected true, got isOwner:%v") - } + require.Truef(t, isOwner, "expected true, got isOwner false") // The test is used to exit campaign loop. d.OwnerManager().Cancel() isOwner = checkOwner(d, false) - if isOwner { - require.Emptyf(t, isOwner, "expected false, got isOwner:%v") - } + require.Falsef(t, isOwner, "expected false, got isOwner:%v") time.Sleep(200 * time.Millisecond) ownerID, _ := manager.GetOwnerID(goctx.Background()) // The error is ok to be not nil since we canceled the manager. - if ownerID != "" { - require.Emptyf(t, ownerID, "owner %s is not empty") - } + require.Emptyf(t, ownerID, "owner %s is not empty", ownerID) } func TestCluster(t *testing.T) { @@ -130,14 +116,10 @@ func TestCluster(t *testing.T) { owner.ManagerSessionTTL = orignalTTL }() store, err := mockstore.NewMockStore() - if err != nil { - require.NoError(t, err) - } + require.NoError(t, err) defer func() { err := store.Close() - if err != nil { - require.NoError(t, err) - } + require.NoError(t, err) }() clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 4}) defer clus.Terminate(t) @@ -153,13 +135,9 @@ func TestCluster(t *testing.T) { WithInfoCache(ic), ) err = d.Start(nil) - if err != nil { - require.NoErrorf(t, err, "DDL start failed %v") - } + require.NoErrorf(t, err, "DDL start failed") isOwner := checkOwner(d, true) - if !isOwner { - require.Emptyf(t, isOwner, "expect true, got isOwner:%v") - } + require.Truef(t, isOwner, "expect true, got isOwner false") cli1 := clus.Client(1) ic2 := infoschema.NewCache(2) ic2.Insert(infoschema.MockInfoSchemaWithSchemaVer(nil, 0), 0) @@ -171,28 +149,18 @@ func TestCluster(t *testing.T) { WithInfoCache(ic2), ) err = d1.Start(nil) - if err != nil { - require.NoErrorf(t, err, "DDL start failed %v") - } + require.NoErrorf(t, err, "DDL start failed") isOwner = checkOwner(d1, false) - if isOwner { - require.Emptyf(t, isOwner, "expect false, got isOwner:%v") - } + require.Falsef(t, isOwner, "expect false, got isOwner true") // Delete the leader key, the d1 become the owner. cliRW := clus.Client(2) err = deleteLeader(cliRW, DDLOwnerKey) - if err != nil { - require.NoError(t, err) - } + require.NoError(t, err) isOwner = checkOwner(d, false) - if isOwner { - require.Emptyf(t, isOwner, "expect false, got isOwner:%v") - } + require.Falsef(t, isOwner, "expect false, got isOwner true") err = d.Stop() - if err != nil { - require.NoErrorf(t, err, "DDL stop failed %v") - } + require.NoErrorf(t, err, "DDL stop failed") // d3 (not owner) stop cli3 := clus.Client(3) ic3 := infoschema.NewCache(2) @@ -205,40 +173,26 @@ func TestCluster(t *testing.T) { WithInfoCache(ic3), ) err = d3.Start(nil) - if err != nil { - require.NoErrorf(t, err, "DDL start failed %v") - } + require.NoErrorf(t, err, "DDL start failed") defer func() { err = d3.Stop() - if err != nil { - require.NoErrorf(t, err, "DDL stop failed %v") - } + require.NoErrorf(t, err, "DDL stop failed") }() isOwner = checkOwner(d3, false) - if isOwner { - require.Emptyf(t, isOwner, "expect false, got isOwner:%v") - } + require.Falsef(t, isOwner, "expect false, got isOwner true") err = d3.Stop() - if err != nil { - require.NoErrorf(t, err, "DDL stop failed %v") - } + require.NoErrorf(t, err, "DDL stop failed") // Cancel the owner context, there is no owner. err = d1.Stop() - if err != nil { - require.NoErrorf(t, err, "DDL stop failed %v") - } + require.NoErrorf(t, err, "DDL stop failed") session, err := concurrency.NewSession(cliRW) - if err != nil { - require.NoErrorf(t, err, "new session failed %v") - } + require.NoErrorf(t, err, "new session failed") elec := concurrency.NewElection(session, DDLOwnerKey) logPrefix := fmt.Sprintf("[ddl] %s ownerManager %s", DDLOwnerKey, "useless id") logCtx := logutil.WithKeyValue(context.Background(), "owner info", logPrefix) _, err = owner.GetOwnerInfo(goctx.Background(), logCtx, elec, "useless id") - if !terror.ErrorEqual(err, concurrency.ErrElectionNoLeader) { - require.NoErrorf(t, err, "get owner info result don't match, err %v") - } + require.Truef(t, terror.ErrorEqual(err, concurrency.ErrElectionNoLeader), "get owner info result don't match, err %v") } func deleteLeader(cli *clientv3.Client, prefixKey string) error { From eea749555b606c030d0b6317f53e3c69122965f6 Mon Sep 17 00:00:00 2001 From: Tenghu Zhang Date: Wed, 4 Aug 2021 14:42:50 +0000 Subject: [PATCH 4/6] remove TestCluster t.Parallel() --- owner/manager_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/owner/manager_test.go b/owner/manager_test.go index af746283e4317..307777866996a 100644 --- a/owner/manager_test.go +++ b/owner/manager_test.go @@ -105,7 +105,6 @@ func TestSingle(t *testing.T) { } func TestCluster(t *testing.T) { - t.Parallel() if runtime.GOOS == "windows" { t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") } From 9b698a43aaf68cf7cfeae92dd9bee31d6193f1c4 Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 5 Aug 2021 11:16:58 +0800 Subject: [PATCH 5/6] fix fail tests Signed-off-by: tison --- owner/fail_test.go | 37 ++++++----- owner/main_test.go | 16 +---- owner/manager_test.go | 141 +++++++++++++++++++++++------------------- 3 files changed, 100 insertions(+), 94 deletions(-) diff --git a/owner/fail_test.go b/owner/fail_test.go index 5ece39047cd06..dff5854423645 100644 --- a/owner/fail_test.go +++ b/owner/fail_test.go @@ -10,6 +10,7 @@ // distributed under the License is distributed on an "AS IS" BASIS, // See the License for the specific language governing permissions and // limitations under the License. + // +build !windows package owner @@ -26,32 +27,34 @@ import ( "github.com/pingcap/failpoint" "github.com/pingcap/parser/terror" - + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.etcd.io/etcd/clientv3" "google.golang.org/grpc" ) var ( - dialTimeout = 5 * time.Second + dialTimeout = 3 * time.Second retryCnt = math.MaxInt32 ) func TestFailNewSession(t *testing.T) { - t.Parallel() - os.Remove("new_session:0") + _ = os.Remove("new_session:0") ln, err := net.Listen("unix", "new_session:0") require.NoError(t, err) + addr := ln.Addr() endpoints := []string{fmt.Sprintf("%s://%s", addr.Network(), addr.String())} - require.Nil(t, err) + require.NoError(t, err) + srv := grpc.NewServer(grpc.ConnectionTimeout(time.Minute)) var stop sync.WaitGroup stop.Add(1) + go func() { + defer stop.Done() err = srv.Serve(ln) - require.Errorf(t, err, "can't serve gRPC requests") - stop.Done() + assert.NoError(t, err) }() defer func() { @@ -67,11 +70,11 @@ func TestFailNewSession(t *testing.T) { require.NoError(t, err) defer func() { if cli != nil { - cli.Close() + _ = cli.Close() } - require.Nil(t, failpoint.Disable("github.com/pingcap/tidb/owner/closeClient")) + require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/owner/closeClient")) }() - require.NoError(t, err, failpoint.Enable("github.com/pingcap/tidb/owner/closeClient", `return(true)`)) + require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/owner/closeClient", `return(true)`)) // TODO: It takes more than 2s here in etcd client, the CI takes 5s to run this test. // The config is hard coded, not way to control it outside. @@ -79,9 +82,9 @@ func TestFailNewSession(t *testing.T) { // https://github.com/etcd-io/etcd/blob/ae9734e/clientv3/concurrency/session.go#L38 // https://github.com/etcd-io/etcd/blob/ae9734ed278b7a1a7dfc82e800471ebbf9fce56f/clientv3/client.go#L253 // https://github.com/etcd-io/etcd/blob/ae9734ed278b7a1a7dfc82e800471ebbf9fce56f/clientv3/retry_interceptor.go#L63 - _, err = NewSession(context.Background(), "fail_new_serssion", cli, retryCnt, ManagerSessionTTL) + _, err = NewSession(context.Background(), "fail_new_session", cli, retryCnt, ManagerSessionTTL) isContextDone := terror.ErrorEqual(grpc.ErrClientConnClosing, err) || terror.ErrorEqual(context.Canceled, err) - require.True(t, isContextDone, "err %v", err) + require.Truef(t, isContextDone, "err %v", err) }() func() { @@ -92,16 +95,16 @@ func TestFailNewSession(t *testing.T) { require.NoError(t, err) defer func() { if cli != nil { - cli.Close() + _ = cli.Close() } - require.Nil(t, failpoint.Disable("github.com/pingcap/tidb/owner/closeGrpc")) + require.NoError(t, failpoint.Disable("github.com/pingcap/tidb/owner/closeGrpc")) }() - require.Nil(t, failpoint.Enable("github.com/pingcap/tidb/owner/closeGrpc", `return(false)`)) + require.NoError(t, failpoint.Enable("github.com/pingcap/tidb/owner/closeGrpc", `return(true)`)) // TODO: It takes more than 2s here in etcd client, the CI takes 5s to run this test. // The config is hard coded, not way to control it outside. - _, err = NewSession(context.Background(), "fail_new_serssion", cli, retryCnt, ManagerSessionTTL) + _, err = NewSession(context.Background(), "fail_new_session", cli, retryCnt, ManagerSessionTTL) isContextDone := terror.ErrorEqual(grpc.ErrClientConnClosing, err) || terror.ErrorEqual(context.Canceled, err) - require.True(t, isContextDone, "err %v", err) + require.Truef(t, isContextDone, "err %v", err) }() } diff --git a/owner/main_test.go b/owner/main_test.go index 33009f392184c..ab9498a86833c 100644 --- a/owner/main_test.go +++ b/owner/main_test.go @@ -14,26 +14,16 @@ package owner import ( - "os" "testing" - "github.com/pingcap/log" - "github.com/pingcap/tidb/util/logutil" "github.com/pingcap/tidb/util/testbridge" "go.uber.org/goleak" - "go.uber.org/zap" ) func TestMain(m *testing.M) { testbridge.WorkaroundGoCheckFlags() - - // Ignore this test on the windows platform, because calling unix socket with address in - // host:port format fails on windows. - logLevel := os.Getenv("log_level") - err := logutil.InitLogger(logutil.NewLogConfig(logLevel, "", "", logutil.EmptyFileLogConfig, false)) - if err != nil { - log.Fatal("put failed", zap.Error(err)) + opts := []goleak.Option{ + goleak.IgnoreTopFunction("go.etcd.io/etcd/pkg/logutil.(*MergeLogger).outputLoop"), } - - goleak.VerifyTestMain(m) + goleak.VerifyTestMain(m, opts...) } diff --git a/owner/manager_test.go b/owner/manager_test.go index 307777866996a..24243b7b52f60 100644 --- a/owner/manager_test.go +++ b/owner/manager_test.go @@ -11,12 +11,13 @@ // See the License for the specific language governing permissions and // limitations under the License. +// +build !windows + package owner_test import ( "context" "fmt" - "runtime" "testing" "time" @@ -36,24 +37,7 @@ import ( const testLease = 5 * time.Millisecond -func checkOwner(d DDL, fbVal bool) (isOwner bool) { - manager := d.OwnerManager() - // The longest to wait for 30 seconds to - // make sure that campaigning owners is completed. - for i := 0; i < 6000; i++ { - time.Sleep(5 * time.Millisecond) - isOwner = manager.IsOwner() - if isOwner == fbVal { - break - } - } - return -} - func TestSingle(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") - } store, err := mockstore.NewMockStore() require.NoError(t, err) defer func() { @@ -61,69 +45,71 @@ func TestSingle(t *testing.T) { require.NoError(t, err) }() - clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 1}) - defer clus.Terminate(t) - cli := clus.RandClient() + cluster := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 1}) + defer cluster.Terminate(t) + + client := cluster.RandClient() ctx := goctx.Background() ic := infoschema.NewCache(2) ic.Insert(infoschema.MockInfoSchemaWithSchemaVer(nil, 0), 0) d := NewDDL( ctx, - WithEtcdClient(cli), + WithEtcdClient(client), WithStore(store), WithLease(testLease), WithInfoCache(ic), ) err = d.Start(nil) - require.NoErrorf(t, err, "DDL start failed") + require.NoError(t, err) defer func() { _ = d.Stop() }() isOwner := checkOwner(d, true) - require.Truef(t, isOwner, "expet true, got isOowner false") + require.True(t, isOwner) // test for newSession failed ctx, cancel := goctx.WithCancel(ctx) - manager := owner.NewOwnerManager(ctx, cli, "ddl", "ddl_id", DDLOwnerKey) + manager := owner.NewOwnerManager(ctx, client, "ddl", "ddl_id", DDLOwnerKey) cancel() + err = manager.CampaignOwner() - if !terror.ErrorEqual(err, goctx.Canceled) && - !terror.ErrorEqual(err, goctx.DeadlineExceeded) { - require.NoErrorf(t, err, "campaigned result don't match, err %v") - } + comment := fmt.Sprintf("campaigned result don't match, err %v", err) + require.True(t, terror.ErrorEqual(err, goctx.Canceled) || terror.ErrorEqual(err, goctx.DeadlineExceeded), comment) + isOwner = checkOwner(d, true) - require.Truef(t, isOwner, "expected true, got isOwner false") + require.True(t, isOwner) + // The test is used to exit campaign loop. d.OwnerManager().Cancel() isOwner = checkOwner(d, false) - require.Falsef(t, isOwner, "expected false, got isOwner:%v") + require.False(t, isOwner) + time.Sleep(200 * time.Millisecond) + + // err is ok to be not nil since we canceled the manager. ownerID, _ := manager.GetOwnerID(goctx.Background()) - // The error is ok to be not nil since we canceled the manager. - require.Emptyf(t, ownerID, "owner %s is not empty", ownerID) + require.Equal(t, "", ownerID) } func TestCluster(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") - } - tmpTTL := 3 - orignalTTL := owner.ManagerSessionTTL - owner.ManagerSessionTTL = tmpTTL + originalTTL := owner.ManagerSessionTTL + owner.ManagerSessionTTL = 3 defer func() { - owner.ManagerSessionTTL = orignalTTL + owner.ManagerSessionTTL = originalTTL }() + store, err := mockstore.NewMockStore() require.NoError(t, err) defer func() { err := store.Close() require.NoError(t, err) }() - clus := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 4}) - defer clus.Terminate(t) - cli := clus.Client(0) + cluster := integration.NewClusterV3(t, &integration.ClusterConfig{Size: 4}) + defer cluster.Terminate(t) + + cli := cluster.Client(0) ic := infoschema.NewCache(2) ic.Insert(infoschema.MockInfoSchemaWithSchemaVer(nil, 0), 0) d := NewDDL( @@ -133,11 +119,14 @@ func TestCluster(t *testing.T) { WithLease(testLease), WithInfoCache(ic), ) + err = d.Start(nil) - require.NoErrorf(t, err, "DDL start failed") + require.NoError(t, err) + isOwner := checkOwner(d, true) - require.Truef(t, isOwner, "expect true, got isOwner false") - cli1 := clus.Client(1) + require.True(t, isOwner) + + cli1 := cluster.Client(1) ic2 := infoschema.NewCache(2) ic2.Insert(infoschema.MockInfoSchemaWithSchemaVer(nil, 0), 0) d1 := NewDDL( @@ -148,20 +137,24 @@ func TestCluster(t *testing.T) { WithInfoCache(ic2), ) err = d1.Start(nil) - require.NoErrorf(t, err, "DDL start failed") + require.NoError(t, err) + isOwner = checkOwner(d1, false) - require.Falsef(t, isOwner, "expect false, got isOwner true") + require.False(t, isOwner) // Delete the leader key, the d1 become the owner. - cliRW := clus.Client(2) + cliRW := cluster.Client(2) err = deleteLeader(cliRW, DDLOwnerKey) require.NoError(t, err) + isOwner = checkOwner(d, false) - require.Falsef(t, isOwner, "expect false, got isOwner true") + require.False(t, isOwner) + err = d.Stop() - require.NoErrorf(t, err, "DDL stop failed") + require.NoError(t, err) + // d3 (not owner) stop - cli3 := clus.Client(3) + cli3 := cluster.Client(3) ic3 := infoschema.NewCache(2) ic3.Insert(infoschema.MockInfoSchemaWithSchemaVer(nil, 0), 0) d3 := NewDDL( @@ -172,26 +165,44 @@ func TestCluster(t *testing.T) { WithInfoCache(ic3), ) err = d3.Start(nil) - require.NoErrorf(t, err, "DDL start failed") + require.NoError(t, err) defer func() { err = d3.Stop() - require.NoErrorf(t, err, "DDL stop failed") + require.NoError(t, err) }() + isOwner = checkOwner(d3, false) - require.Falsef(t, isOwner, "expect false, got isOwner true") + require.False(t, isOwner) + err = d3.Stop() - require.NoErrorf(t, err, "DDL stop failed") + require.NoError(t, err) // Cancel the owner context, there is no owner. err = d1.Stop() - require.NoErrorf(t, err, "DDL stop failed") + require.NoError(t, err) + session, err := concurrency.NewSession(cliRW) - require.NoErrorf(t, err, "new session failed") - elec := concurrency.NewElection(session, DDLOwnerKey) + require.NoError(t, err) + + election := concurrency.NewElection(session, DDLOwnerKey) logPrefix := fmt.Sprintf("[ddl] %s ownerManager %s", DDLOwnerKey, "useless id") logCtx := logutil.WithKeyValue(context.Background(), "owner info", logPrefix) - _, err = owner.GetOwnerInfo(goctx.Background(), logCtx, elec, "useless id") - require.Truef(t, terror.ErrorEqual(err, concurrency.ErrElectionNoLeader), "get owner info result don't match, err %v") + _, err = owner.GetOwnerInfo(goctx.Background(), logCtx, election, "useless id") + require.Truef(t, terror.ErrorEqual(err, concurrency.ErrElectionNoLeader), "get owner info result don't match, err %v", err) +} + +func checkOwner(d DDL, fbVal bool) (isOwner bool) { + manager := d.OwnerManager() + // The longest to wait for 30 seconds to + // make sure that campaigning owners is completed. + for i := 0; i < 6000; i++ { + time.Sleep(5 * time.Millisecond) + isOwner = manager.IsOwner() + if isOwner == fbVal { + break + } + } + return } func deleteLeader(cli *clientv3.Client, prefixKey string) error { @@ -199,9 +210,11 @@ func deleteLeader(cli *clientv3.Client, prefixKey string) error { if err != nil { return errors.Trace(err) } - defer session.Close() - elec := concurrency.NewElection(session, prefixKey) - resp, err := elec.Leader(goctx.Background()) + defer func() { + _ = session.Close() + }() + election := concurrency.NewElection(session, prefixKey) + resp, err := election.Leader(goctx.Background()) if err != nil { return errors.Trace(err) } From 1ea7c197e11092feb24da9aa0e5f477ab9629899 Mon Sep 17 00:00:00 2001 From: tison Date: Thu, 5 Aug 2021 13:32:07 +0800 Subject: [PATCH 6/6] replace conditional compile with skip Signed-off-by: tison --- owner/fail_test.go | 7 +++++-- owner/manager_test.go | 11 +++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/owner/fail_test.go b/owner/fail_test.go index dff5854423645..4ce291af10cbb 100644 --- a/owner/fail_test.go +++ b/owner/fail_test.go @@ -11,8 +11,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -// +build !windows - package owner import ( @@ -21,6 +19,7 @@ import ( "math" "net" "os" + "runtime" "sync" "testing" "time" @@ -39,6 +38,10 @@ var ( ) func TestFailNewSession(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") + } + _ = os.Remove("new_session:0") ln, err := net.Listen("unix", "new_session:0") require.NoError(t, err) diff --git a/owner/manager_test.go b/owner/manager_test.go index 24243b7b52f60..9f923437ea73f 100644 --- a/owner/manager_test.go +++ b/owner/manager_test.go @@ -11,13 +11,12 @@ // See the License for the specific language governing permissions and // limitations under the License. -// +build !windows - package owner_test import ( "context" "fmt" + "runtime" "testing" "time" @@ -38,6 +37,10 @@ import ( const testLease = 5 * time.Millisecond func TestSingle(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") + } + store, err := mockstore.NewMockStore() require.NoError(t, err) defer func() { @@ -93,6 +96,10 @@ func TestSingle(t *testing.T) { } func TestCluster(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("integration.NewClusterV3 will create file contains a colon which is not allowed on Windows") + } + originalTTL := owner.ManagerSessionTTL owner.ManagerSessionTTL = 3 defer func() {