From 4e322476122433a15a02cf6172f75f803bdfb64c Mon Sep 17 00:00:00 2001 From: yuyawk Date: Thu, 7 Oct 2021 23:20:45 +0900 Subject: [PATCH 1/5] Refactor: migrate test infra for br/pkg/logutil/loggin_test.go to testify --- br/pkg/logutil/logging_test.go | 122 ++++++++++----------------------- 1 file changed, 35 insertions(+), 87 deletions(-) diff --git a/br/pkg/logutil/logging_test.go b/br/pkg/logutil/logging_test.go index 6522ea7c88db9..196e7e4280b85 100644 --- a/br/pkg/logutil/logging_test.go +++ b/br/pkg/logutil/logging_test.go @@ -5,12 +5,10 @@ package logutil_test import ( "context" "fmt" - "math" "strings" "testing" "time" - . "github.com/pingcap/check" "github.com/pingcap/errors" backuppb "github.com/pingcap/kvproto/pkg/brpb" "github.com/pingcap/kvproto/pkg/import_sstpb" @@ -18,24 +16,18 @@ import ( berrors "github.com/pingcap/tidb/br/pkg/errors" "github.com/pingcap/tidb/br/pkg/logutil" "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/zap" "go.uber.org/zap/zapcore" "go.uber.org/zap/zaptest/observer" ) -func Test(t *testing.T) { - TestingT(t) -} - -var _ = Suite(&testLoggingSuite{}) - -type testLoggingSuite struct{} - -func assertTrimEqual(c *C, f zapcore.Field, expect string) { +func assertTrimEqual(t *testing.T, f zapcore.Field, expect string) { encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{f}) - c.Assert(err, IsNil) - c.Assert(strings.TrimRight(out.String(), "\n"), Equals, expect) + require.NoError(t, err) + assert.Equal(t, strings.TrimRight(out.String(), "\n"), expect) } func newFile(j int) *backuppb.File { @@ -54,29 +46,7 @@ func newFile(j int) *backuppb.File { } } -type isAbout struct{} - -func (isAbout) Info() *CheckerInfo { - return &CheckerInfo{ - Name: "isAbout", - Params: []string{ - "actual", - "expect", - }, - } -} - -func (isAbout) Check(params []interface{}, names []string) (result bool, error string) { - actual := params[0].(float64) - expect := params[1].(float64) - - if diff := math.Abs(1 - (actual / expect)); diff > 0.1 { - return false, fmt.Sprintf("The diff(%.2f) between actual(%.2f) and expect(%.2f) is too huge.", diff, actual, expect) - } - return true, "" -} - -func (s *testLoggingSuite) TestRater(c *C) { +func TestRater(t *testing.T) { m := prometheus.NewCounter(prometheus.CounterOpts{ Namespace: "testing", Name: "rater", @@ -87,19 +57,19 @@ func (s *testLoggingSuite) TestRater(c *C) { rater := logutil.TraceRateOver(m) timePass := time.Now() rater.Inc() - c.Assert(rater.RateAt(timePass.Add(100*time.Millisecond)), isAbout{}, 10.0) + assert.InEpsilon(t, rater.RateAt(timePass.Add(100*time.Millisecond)), 10.0, 0.1) rater.Inc() - c.Assert(rater.RateAt(timePass.Add(150*time.Millisecond)), isAbout{}, 13.0) + assert.InEpsilon(t, rater.RateAt(timePass.Add(150*time.Millisecond)), 13.0, 0.1) rater.Add(18) - c.Assert(rater.RateAt(timePass.Add(200*time.Millisecond)), isAbout{}, 100.0) + assert.InEpsilon(t, rater.RateAt(timePass.Add(200*time.Millisecond)), 100.0, 0.1) } -func (s *testLoggingSuite) TestFile(c *C) { - assertTrimEqual(c, logutil.File(newFile(1)), +func TestFile(t *testing.T) { + assertTrimEqual(t, logutil.File(newFile(1)), `{"file": {"name": "1", "CF": "write", "sha256": "31", "startKey": "31", "endKey": "32", "startVersion": 1, "endVersion": 2, "totalKvs": 1, "totalBytes": 1, "CRC64Xor": 1}}`) } -func (s *testLoggingSuite) TestFiles(c *C) { +func TestFiles(t *testing.T) { cases := []struct { count int expect string @@ -119,18 +89,18 @@ func (s *testLoggingSuite) TestFiles(c *C) { for j := 0; j < cs.count; j++ { ranges[j] = newFile(j) } - assertTrimEqual(c, logutil.Files(ranges), cs.expect) + assertTrimEqual(t, logutil.Files(ranges), cs.expect) } } -func (s *testLoggingSuite) TestKey(c *C) { +func TestKey(t *testing.T) { encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{logutil.Key("test", []byte{0, 1, 2, 3})}) - c.Assert(err, IsNil) - c.Assert(strings.Trim(out.String(), "\n"), Equals, `{"test": "00010203"}`) + require.NoError(t, err) + assert.Equal(t, strings.Trim(out.String(), "\n"), `{"test": "00010203"}`) } -func (s *testLoggingSuite) TestKeys(c *C) { +func TestKeys(t *testing.T) { cases := []struct { count int expect string @@ -150,11 +120,11 @@ func (s *testLoggingSuite) TestKeys(c *C) { for j := 0; j < cs.count; j++ { keys[j] = []byte(fmt.Sprintf("%04d", j)) } - assertTrimEqual(c, logutil.Keys(keys), cs.expect) + assertTrimEqual(t, logutil.Keys(keys), cs.expect) } } -func (s *testLoggingSuite) TestRewriteRule(c *C) { +func TestRewriteRule(t *testing.T) { rule := &import_sstpb.RewriteRule{ OldKeyPrefix: []byte("old"), NewKeyPrefix: []byte("new"), @@ -163,11 +133,11 @@ func (s *testLoggingSuite) TestRewriteRule(c *C) { encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{logutil.RewriteRule(rule)}) - c.Assert(err, IsNil) - c.Assert(strings.Trim(out.String(), "\n"), Equals, `{"rewriteRule": {"oldKeyPrefix": "6f6c64", "newKeyPrefix": "6e6577", "newTimestamp": 5592405}}`) + require.NoError(t, err) + assert.Equal(t, strings.Trim(out.String(), "\n"), `{"rewriteRule": {"oldKeyPrefix": "6f6c64", "newKeyPrefix": "6e6577", "newTimestamp": 5592405}}`) } -func (s *testLoggingSuite) TestRegion(c *C) { +func TestRegion(t *testing.T) { region := &metapb.Region{ Id: 1, StartKey: []byte{0x00, 0x01}, @@ -176,17 +146,17 @@ func (s *testLoggingSuite) TestRegion(c *C) { Peers: []*metapb.Peer{{Id: 2, StoreId: 3}, {Id: 4, StoreId: 5}}, } - assertTrimEqual(c, logutil.Region(region), + assertTrimEqual(t, logutil.Region(region), `{"region": {"ID": 1, "startKey": "0001", "endKey": "0002", "epoch": "conf_ver:1 version:1 ", "peers": "id:2 store_id:3 ,id:4 store_id:5 "}}`) } -func (s *testLoggingSuite) TestLeader(c *C) { +func TestLeader(t *testing.T) { leader := &metapb.Peer{Id: 2, StoreId: 3} - assertTrimEqual(c, logutil.Leader(leader), `{"leader": "id:2 store_id:3 "}`) + assertTrimEqual(t, logutil.Leader(leader), `{"leader": "id:2 store_id:3 "}`) } -func (s *testLoggingSuite) TestSSTMeta(c *C) { +func TestSSTMeta(t *testing.T) { meta := &import_sstpb.SSTMeta{ Uuid: []byte("mock uuid"), Range: &import_sstpb.Range{ @@ -200,39 +170,17 @@ func (s *testLoggingSuite) TestSSTMeta(c *C) { RegionEpoch: &metapb.RegionEpoch{ConfVer: 1, Version: 1}, } - assertTrimEqual(c, logutil.SSTMeta(meta), + assertTrimEqual(t, logutil.SSTMeta(meta), `{"sstMeta": {"CF": "default", "endKeyExclusive": false, "CRC32": 5592405, "length": 1, "regionID": 1, "regionEpoch": "conf_ver:1 version:1 ", "startKey": "0001", "endKey": "0002", "UUID": "invalid UUID 6d6f636b2075756964"}}`) } -func (s *testLoggingSuite) TestShortError(c *C) { +func TestShortError(t *testing.T) { err := errors.Annotate(berrors.ErrInvalidArgument, "test") - assertTrimEqual(c, logutil.ShortError(err), `{"error": "test: [BR:Common:ErrInvalidArgument]invalid argument"}`) -} - -type FieldEquals struct{} - -func (f FieldEquals) Info() *CheckerInfo { - return &CheckerInfo{ - Name: "FieldEquals", - Params: []string{ - "expected", - "actual", - }, - } -} - -func (f FieldEquals) Check(params []interface{}, names []string) (result bool, err string) { - expected := params[0].(zap.Field) - actual := params[1].(zap.Field) - - if !expected.Equals(actual) { - return false, "Field not match." - } - return true, "" + assertTrimEqual(t, logutil.ShortError(err), `{"error": "test: [BR:Common:ErrInvalidArgument]invalid argument"}`) } -func (s *testLoggingSuite) TestContextual(c *C) { +func TestContextual(t *testing.T) { testCore, logs := observer.New(zap.InfoLevel) logutil.ResetGlobalLogger(zap.New(testCore)) @@ -244,15 +192,15 @@ func (s *testLoggingSuite) TestContextual(c *C) { l.Info("let's go!", zap.String("character", "solte")) observedLogs := logs.TakeAll() - checkLog(c, observedLogs[0], + checkLog(t, observedLogs[0], "going to take an adventure?", zap.Int("HP", 50), zap.Int("HP-MAX", 50), zap.String("character", "solte")) - checkLog(c, observedLogs[1], + checkLog(t, observedLogs[1], "let's go!", zap.Strings("friends", []string{"firo", "seren", "black"}), zap.String("character", "solte")) } -func checkLog(c *C, actual observer.LoggedEntry, message string, fields ...zap.Field) { - c.Assert(message, Equals, actual.Message) +func checkLog(t *testing.T, actual observer.LoggedEntry, message string, fields ...zap.Field) { + assert.Equal(t, message, actual.Message) for i, f := range fields { - c.Assert(f, FieldEquals{}, actual.Context[i]) + assert.Equal(t, f, actual.Context[i]) } } From fd48da3c5e7f7e692e93eb5ed13b4c6110923b3a Mon Sep 17 00:00:00 2001 From: yuyawk Date: Fri, 8 Oct 2021 11:51:47 +0900 Subject: [PATCH 2/5] Suggestions reflected: * make t.Parallel() available * testify/assert -> testify/require * fix the order of the argments in require.(function name) --- br/pkg/logutil/logging_test.go | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/br/pkg/logutil/logging_test.go b/br/pkg/logutil/logging_test.go index 196e7e4280b85..827e2d5dab558 100644 --- a/br/pkg/logutil/logging_test.go +++ b/br/pkg/logutil/logging_test.go @@ -16,7 +16,6 @@ import ( berrors "github.com/pingcap/tidb/br/pkg/errors" "github.com/pingcap/tidb/br/pkg/logutil" "github.com/prometheus/client_golang/prometheus" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/zap" "go.uber.org/zap/zapcore" @@ -27,7 +26,7 @@ func assertTrimEqual(t *testing.T, f zapcore.Field, expect string) { encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{f}) require.NoError(t, err) - assert.Equal(t, strings.TrimRight(out.String(), "\n"), expect) + require.Equal(t, expect, strings.TrimRight(out.String(), "\n")) } func newFile(j int) *backuppb.File { @@ -47,6 +46,7 @@ func newFile(j int) *backuppb.File { } func TestRater(t *testing.T) { + t.Parallel() m := prometheus.NewCounter(prometheus.CounterOpts{ Namespace: "testing", Name: "rater", @@ -57,19 +57,21 @@ func TestRater(t *testing.T) { rater := logutil.TraceRateOver(m) timePass := time.Now() rater.Inc() - assert.InEpsilon(t, rater.RateAt(timePass.Add(100*time.Millisecond)), 10.0, 0.1) + require.InEpsilon(t, 10.0, rater.RateAt(timePass.Add(100*time.Millisecond)), 0.1) rater.Inc() - assert.InEpsilon(t, rater.RateAt(timePass.Add(150*time.Millisecond)), 13.0, 0.1) + require.InEpsilon(t, 13.0, rater.RateAt(timePass.Add(150*time.Millisecond)), 0.1) rater.Add(18) - assert.InEpsilon(t, rater.RateAt(timePass.Add(200*time.Millisecond)), 100.0, 0.1) + require.InEpsilon(t, 100.0, rater.RateAt(timePass.Add(200*time.Millisecond)), 0.1) } func TestFile(t *testing.T) { + t.Parallel() assertTrimEqual(t, logutil.File(newFile(1)), `{"file": {"name": "1", "CF": "write", "sha256": "31", "startKey": "31", "endKey": "32", "startVersion": 1, "endVersion": 2, "totalKvs": 1, "totalBytes": 1, "CRC64Xor": 1}}`) } func TestFiles(t *testing.T) { + t.Parallel() cases := []struct { count int expect string @@ -94,13 +96,15 @@ func TestFiles(t *testing.T) { } func TestKey(t *testing.T) { + t.Parallel() encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{logutil.Key("test", []byte{0, 1, 2, 3})}) require.NoError(t, err) - assert.Equal(t, strings.Trim(out.String(), "\n"), `{"test": "00010203"}`) + require.Equal(t, `{"test": "00010203"}`, strings.Trim(out.String(), "\n")) } func TestKeys(t *testing.T) { + t.Parallel() cases := []struct { count int expect string @@ -125,6 +129,7 @@ func TestKeys(t *testing.T) { } func TestRewriteRule(t *testing.T) { + t.Parallel() rule := &import_sstpb.RewriteRule{ OldKeyPrefix: []byte("old"), NewKeyPrefix: []byte("new"), @@ -134,10 +139,11 @@ func TestRewriteRule(t *testing.T) { encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{logutil.RewriteRule(rule)}) require.NoError(t, err) - assert.Equal(t, strings.Trim(out.String(), "\n"), `{"rewriteRule": {"oldKeyPrefix": "6f6c64", "newKeyPrefix": "6e6577", "newTimestamp": 5592405}}`) + require.Equal(t, `{"rewriteRule": {"oldKeyPrefix": "6f6c64", "newKeyPrefix": "6e6577", "newTimestamp": 5592405}}`, strings.Trim(out.String(), "\n")) } func TestRegion(t *testing.T) { + t.Parallel() region := &metapb.Region{ Id: 1, StartKey: []byte{0x00, 0x01}, @@ -151,12 +157,14 @@ func TestRegion(t *testing.T) { } func TestLeader(t *testing.T) { + t.Parallel() leader := &metapb.Peer{Id: 2, StoreId: 3} assertTrimEqual(t, logutil.Leader(leader), `{"leader": "id:2 store_id:3 "}`) } func TestSSTMeta(t *testing.T) { + t.Parallel() meta := &import_sstpb.SSTMeta{ Uuid: []byte("mock uuid"), Range: &import_sstpb.Range{ @@ -175,12 +183,14 @@ func TestSSTMeta(t *testing.T) { } func TestShortError(t *testing.T) { + t.Parallel() err := errors.Annotate(berrors.ErrInvalidArgument, "test") assertTrimEqual(t, logutil.ShortError(err), `{"error": "test: [BR:Common:ErrInvalidArgument]invalid argument"}`) } func TestContextual(t *testing.T) { + t.Parallel() testCore, logs := observer.New(zap.InfoLevel) logutil.ResetGlobalLogger(zap.New(testCore)) @@ -199,8 +209,8 @@ func TestContextual(t *testing.T) { } func checkLog(t *testing.T, actual observer.LoggedEntry, message string, fields ...zap.Field) { - assert.Equal(t, message, actual.Message) + require.Equal(t, message, actual.Message) for i, f := range fields { - assert.Equal(t, f, actual.Context[i]) + require.Equal(t, f, actual.Context[i]) } } From 3c1255deda315dd75c48deff814aa8b481560f49 Mon Sep 17 00:00:00 2001 From: yuyawk Date: Fri, 8 Oct 2021 14:49:34 +0900 Subject: [PATCH 3/5] Suggestion reflected: use JsonEq to validate JSON string --- br/pkg/logutil/logging_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/br/pkg/logutil/logging_test.go b/br/pkg/logutil/logging_test.go index 827e2d5dab558..157388a1eb949 100644 --- a/br/pkg/logutil/logging_test.go +++ b/br/pkg/logutil/logging_test.go @@ -26,7 +26,7 @@ func assertTrimEqual(t *testing.T, f zapcore.Field, expect string) { encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{f}) require.NoError(t, err) - require.Equal(t, expect, strings.TrimRight(out.String(), "\n")) + require.JSONEq(t, expect, strings.TrimRight(out.String(), "\n")) } func newFile(j int) *backuppb.File { @@ -100,7 +100,7 @@ func TestKey(t *testing.T) { encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{logutil.Key("test", []byte{0, 1, 2, 3})}) require.NoError(t, err) - require.Equal(t, `{"test": "00010203"}`, strings.Trim(out.String(), "\n")) + require.JSONEq(t, `{"test": "00010203"}`, strings.Trim(out.String(), "\n")) } func TestKeys(t *testing.T) { @@ -139,7 +139,7 @@ func TestRewriteRule(t *testing.T) { encoder := zapcore.NewConsoleEncoder(zapcore.EncoderConfig{}) out, err := encoder.EncodeEntry(zapcore.Entry{}, []zap.Field{logutil.RewriteRule(rule)}) require.NoError(t, err) - require.Equal(t, `{"rewriteRule": {"oldKeyPrefix": "6f6c64", "newKeyPrefix": "6e6577", "newTimestamp": 5592405}}`, strings.Trim(out.String(), "\n")) + require.JSONEq(t, `{"rewriteRule": {"oldKeyPrefix": "6f6c64", "newKeyPrefix": "6e6577", "newTimestamp": 5592405}}`, strings.Trim(out.String(), "\n")) } func TestRegion(t *testing.T) { From 1eba6933a2558a71525d3c67ce951c304346367c Mon Sep 17 00:00:00 2001 From: yuyawk Date: Fri, 8 Oct 2021 15:11:21 +0900 Subject: [PATCH 4/5] Suggestion reflected: use Field.Equals instead of require.Equal --- br/pkg/logutil/logging_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/br/pkg/logutil/logging_test.go b/br/pkg/logutil/logging_test.go index 157388a1eb949..aed6ba76420d2 100644 --- a/br/pkg/logutil/logging_test.go +++ b/br/pkg/logutil/logging_test.go @@ -211,6 +211,6 @@ func TestContextual(t *testing.T) { func checkLog(t *testing.T, actual observer.LoggedEntry, message string, fields ...zap.Field) { require.Equal(t, message, actual.Message) for i, f := range fields { - require.Equal(t, f, actual.Context[i]) + require.True(t, f.Equals(actual.Context[i])) } } From acf40b22d3f68bcc337f4e609c5cd7d29745dc34 Mon Sep 17 00:00:00 2001 From: yuyawk Date: Fri, 8 Oct 2021 15:25:42 +0900 Subject: [PATCH 5/5] use Truef to show details about failure --- br/pkg/logutil/logging_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/br/pkg/logutil/logging_test.go b/br/pkg/logutil/logging_test.go index aed6ba76420d2..28ed6a8f708b2 100644 --- a/br/pkg/logutil/logging_test.go +++ b/br/pkg/logutil/logging_test.go @@ -211,6 +211,6 @@ func TestContextual(t *testing.T) { func checkLog(t *testing.T, actual observer.LoggedEntry, message string, fields ...zap.Field) { require.Equal(t, message, actual.Message) for i, f := range fields { - require.True(t, f.Equals(actual.Context[i])) + require.Truef(t, f.Equals(actual.Context[i]), "Expected field(%+v) does not equal to actual one(%+v).", f, actual.Context[i]) } }