From 0b721b9ee243b0754bb77a470027bbe0d792a384 Mon Sep 17 00:00:00 2001 From: hlts2 Date: Mon, 20 Jul 2020 18:36:12 +0900 Subject: [PATCH 1/6] fix: refactoring Signed-off-by: hlts2 --- internal/db/kvs/redis/option.go | 8 ++++ internal/db/kvs/redis/redis.go | 65 +++++++++++++++++++++------------ 2 files changed, 49 insertions(+), 24 deletions(-) diff --git a/internal/db/kvs/redis/option.go b/internal/db/kvs/redis/option.go index 5b30a427b5..596bd91e8c 100644 --- a/internal/db/kvs/redis/option.go +++ b/internal/db/kvs/redis/option.go @@ -33,6 +33,7 @@ var ( defaultOpts = []Option{ WithInitialPingDuration("30ms"), WithInitialPingTimeLimit("5m"), + WithPingFlag(true), } ) @@ -322,3 +323,10 @@ func WithInitialPingDuration(dur string) Option { return nil } } + +func WithPingFlag(flag bool) Option { + return func(r *redisClient) error { + r.pingEnabled = flag + return nil + } +} diff --git a/internal/db/kvs/redis/redis.go b/internal/db/kvs/redis/redis.go index f750a05233..44a5e25463 100644 --- a/internal/db/kvs/redis/redis.go +++ b/internal/db/kvs/redis/redis.go @@ -29,9 +29,11 @@ import ( ) var ( + // Nil is a type alias of redis.Nil. Nil = redis.Nil ) +// Redis is an interface to manipulate Redis server. type Redis interface { TxPipeline() redis.Pipeliner Ping() *StatusCmd @@ -42,10 +44,16 @@ type Redis interface { Deleter } -type Conn = redis.Conn -type IntCmd = redis.IntCmd -type StringCmd = redis.StringCmd -type StatusCmd = redis.StatusCmd +type ( + // Conn is a type alias of redis.Conn. + Conn = redis.Conn + // IntCmd is a type alias of redis.IntCmd. + IntCmd = redis.IntCmd + // StringCmd is a type alias of redis.StringCmd. + StringCmd = redis.StringCmd + // StatusCmd is a type alias of redis.StatusCmd. + StatusCmd = redis.StatusCmd +) type redisClient struct { addrs []string @@ -75,8 +83,12 @@ type redisClient struct { routeRandomly bool tlsConfig *tls.Config writeTimeout time.Duration + + client Redis + pingEnabled bool } +// New returns Redis implementation if no error occurs. func New(ctx context.Context, opts ...Option) (rc Redis, err error) { r := new(redisClient) for _, opt := range append(defaultOpts, opts...) { @@ -84,6 +96,7 @@ func New(ctx context.Context, opts ...Option) (rc Redis, err error) { return nil, errors.ErrOptionFailed(err, reflect.ValueOf(opt)) } } + switch len(r.addrs) { case 0: return nil, errors.ErrRedisAddrsNotFound @@ -91,7 +104,7 @@ func New(ctx context.Context, opts ...Option) (rc Redis, err error) { if len(r.addrs[0]) == 0 { return nil, errors.ErrRedisAddrsNotFound } - rc = redis.NewClient(&redis.Options{ + r.client = redis.NewClient(&redis.Options{ Addr: r.addrs[0], Password: r.password, Dialer: r.dialer, @@ -112,7 +125,7 @@ func New(ctx context.Context, opts ...Option) (rc Redis, err error) { TLSConfig: r.tlsConfig, }) default: - rc = redis.NewClusterClient(&redis.ClusterOptions{ + r.client = redis.NewClusterClient(&redis.ClusterOptions{ Addrs: r.addrs, Dialer: r.dialer, MaxRedirects: r.maxRedirects, @@ -139,25 +152,29 @@ func New(ctx context.Context, opts ...Option) (rc Redis, err error) { }).WithContext(ctx) } - err = func() (err error) { - pctx, cancel := context.WithTimeout(ctx, r.initialPingTimeLimit) - defer cancel() - tick := time.NewTicker(r.initialPingDuration) - for { - select { - case <-pctx.Done(): - return errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), ctx.Err().Error()) - case <-tick.C: - err = rc.Ping().Err() - if err == nil { - return nil - } - log.Error(err) + if r.pingEnabled { + if err = r.ping(ctx); err != nil { + return nil, err + } + } + + return r.client, nil +} + +func (rc *redisClient) ping(ctx context.Context) (err error) { + pctx, cancel := context.WithTimeout(ctx, rc.initialPingTimeLimit) + defer cancel() + tick := time.NewTicker(rc.initialPingDuration) + for { + select { + case <-pctx.Done(): + return errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), pctx.Err().Error()) + case <-tick.C: + err = rc.client.Ping().Err() + if err == nil { + return nil } + log.Error(err) } - }() - if err != nil { - return nil, err } - return rc, nil } From 959c5641d3799736bc6760af0fdd94c6d21b7e95 Mon Sep 17 00:00:00 2001 From: hlts2 Date: Tue, 21 Jul 2020 11:46:41 +0900 Subject: [PATCH 2/6] fix: apply gotests Signed-off-by: hlts2 --- internal/db/kvs/redis/option_test.go | 115 ++++++++++++++- internal/db/kvs/redis/redis_test.go | 201 ++++++++++++++++++++++++++- 2 files changed, 314 insertions(+), 2 deletions(-) diff --git a/internal/db/kvs/redis/option_test.go b/internal/db/kvs/redis/option_test.go index fd48aa04d5..d8c009ce91 100644 --- a/internal/db/kvs/redis/option_test.go +++ b/internal/db/kvs/redis/option_test.go @@ -24,7 +24,6 @@ import ( redis "github.com/go-redis/redis/v7" "github.com/vdaas/vald/internal/net" - "go.uber.org/goleak" ) @@ -3078,3 +3077,117 @@ func TestWithInitialPingDuration(t *testing.T) { }) } } + +func TestWithPingFlag(t *testing.T) { + // Change interface type to the type of object you are testing + type T = interface{} + type args struct { + flag bool + } + type want struct { + obj *T + // Uncomment this line if the option returns an error, otherwise delete it + // err error + } + type test struct { + name string + args args + want want + // Use the first line if the option returns an error. otherwise use the second line + // checkFunc func(want, *T, error) error + // checkFunc func(want, *T) error + beforeFunc func(args) + afterFunc func(args) + } + + // Uncomment this block if the option returns an error, otherwise delete it + /* + defaultCheckFunc := func(w want, obj *T, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if !reflect.DeepEqual(obj, w.obj) { + return errors.Errorf("got = %v, want %v", obj, w.obj) + } + return nil + } + */ + + // Uncomment this block if the option do not returns an error, otherwise delete it + /* + defaultCheckFunc := func(w want, obj *T) error { + if !reflect.DeepEqual(obj, w.obj) { + return errors.Errorf("got = %v, want %v", obj, w.obj) + } + return nil + } + */ + + tests := []test{ + // TODO test cases + /* + { + name: "test_case_1", + args: args { + flag: false, + }, + want: want { + obj: new(T), + }, + }, + */ + + // TODO test cases + /* + func() test { + return test { + name: "test_case_2", + args: args { + flag: false, + }, + want: want { + obj: new(T), + }, + } + }(), + */ + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + defer goleak.VerifyNone(tt) + if test.beforeFunc != nil { + test.beforeFunc(test.args) + } + if test.afterFunc != nil { + defer test.afterFunc(test.args) + } + + // Uncomment this block if the option returns an error, otherwise delete it + /* + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + + got := WithPingFlag(test.args.flag) + obj := new(T) + if err := test.checkFunc(test.want, obj, got(obj)); err != nil { + tt.Errorf("error = %v", err) + } + */ + + // Uncomment this block if the option do not return an error, otherwise delete it + /* + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + got := WithPingFlag(test.args.flag) + obj := new(T) + got(obj) + if err := test.checkFunc(test.want, obj); err != nil { + tt.Errorf("error = %v", err) + } + */ + }) + } +} diff --git a/internal/db/kvs/redis/redis_test.go b/internal/db/kvs/redis/redis_test.go index 7025b1e209..a76690aee0 100644 --- a/internal/db/kvs/redis/redis_test.go +++ b/internal/db/kvs/redis/redis_test.go @@ -18,11 +18,14 @@ package redis import ( "context" + "crypto/tls" "reflect" "testing" + "time" + redis "github.com/go-redis/redis/v7" "github.com/vdaas/vald/internal/errors" - + "github.com/vdaas/vald/internal/net" "go.uber.org/goleak" ) @@ -103,3 +106,199 @@ func TestNew(t *testing.T) { }) } } + +func Test_redisClient_ping(t *testing.T) { + type args struct { + ctx context.Context + } + type fields struct { + addrs []string + clusterSlots func() ([]redis.ClusterSlot, error) + db int + dialTimeout time.Duration + dialer func(ctx context.Context, network, addr string) (net.Conn, error) + idleCheckFrequency time.Duration + idleTimeout time.Duration + initialPingDuration time.Duration + initialPingTimeLimit time.Duration + keyPref string + maxConnAge time.Duration + maxRedirects int + maxRetries int + maxRetryBackoff time.Duration + minIdleConns int + minRetryBackoff time.Duration + onConnect func(*redis.Conn) error + onNewNode func(*redis.Client) + password string + poolSize int + poolTimeout time.Duration + readOnly bool + readTimeout time.Duration + routeByLatency bool + routeRandomly bool + tlsConfig *tls.Config + writeTimeout time.Duration + client Redis + pingEnabled bool + } + type want struct { + err error + } + type test struct { + name string + args args + fields fields + want want + checkFunc func(want, error) error + beforeFunc func(args) + afterFunc func(args) + } + defaultCheckFunc := func(w want, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + return nil + } + tests := []test{ + // TODO test cases + /* + { + name: "test_case_1", + args: args { + ctx: nil, + }, + fields: fields { + addrs: nil, + clusterSlots: nil, + db: 0, + dialTimeout: nil, + dialer: nil, + idleCheckFrequency: nil, + idleTimeout: nil, + initialPingDuration: nil, + initialPingTimeLimit: nil, + keyPref: "", + maxConnAge: nil, + maxRedirects: 0, + maxRetries: 0, + maxRetryBackoff: nil, + minIdleConns: 0, + minRetryBackoff: nil, + onConnect: nil, + onNewNode: nil, + password: "", + poolSize: 0, + poolTimeout: nil, + readOnly: false, + readTimeout: nil, + routeByLatency: false, + routeRandomly: false, + tlsConfig: nil, + writeTimeout: nil, + client: nil, + pingEnabled: false, + }, + want: want{}, + checkFunc: defaultCheckFunc, + }, + */ + + // TODO test cases + /* + func() test { + return test { + name: "test_case_2", + args: args { + ctx: nil, + }, + fields: fields { + addrs: nil, + clusterSlots: nil, + db: 0, + dialTimeout: nil, + dialer: nil, + idleCheckFrequency: nil, + idleTimeout: nil, + initialPingDuration: nil, + initialPingTimeLimit: nil, + keyPref: "", + maxConnAge: nil, + maxRedirects: 0, + maxRetries: 0, + maxRetryBackoff: nil, + minIdleConns: 0, + minRetryBackoff: nil, + onConnect: nil, + onNewNode: nil, + password: "", + poolSize: 0, + poolTimeout: nil, + readOnly: false, + readTimeout: nil, + routeByLatency: false, + routeRandomly: false, + tlsConfig: nil, + writeTimeout: nil, + client: nil, + pingEnabled: false, + }, + want: want{}, + checkFunc: defaultCheckFunc, + } + }(), + */ + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + defer goleak.VerifyNone(tt) + if test.beforeFunc != nil { + test.beforeFunc(test.args) + } + if test.afterFunc != nil { + defer test.afterFunc(test.args) + } + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + rc := &redisClient{ + addrs: test.fields.addrs, + clusterSlots: test.fields.clusterSlots, + db: test.fields.db, + dialTimeout: test.fields.dialTimeout, + dialer: test.fields.dialer, + idleCheckFrequency: test.fields.idleCheckFrequency, + idleTimeout: test.fields.idleTimeout, + initialPingDuration: test.fields.initialPingDuration, + initialPingTimeLimit: test.fields.initialPingTimeLimit, + keyPref: test.fields.keyPref, + maxConnAge: test.fields.maxConnAge, + maxRedirects: test.fields.maxRedirects, + maxRetries: test.fields.maxRetries, + maxRetryBackoff: test.fields.maxRetryBackoff, + minIdleConns: test.fields.minIdleConns, + minRetryBackoff: test.fields.minRetryBackoff, + onConnect: test.fields.onConnect, + onNewNode: test.fields.onNewNode, + password: test.fields.password, + poolSize: test.fields.poolSize, + poolTimeout: test.fields.poolTimeout, + readOnly: test.fields.readOnly, + readTimeout: test.fields.readTimeout, + routeByLatency: test.fields.routeByLatency, + routeRandomly: test.fields.routeRandomly, + tlsConfig: test.fields.tlsConfig, + writeTimeout: test.fields.writeTimeout, + client: test.fields.client, + pingEnabled: test.fields.pingEnabled, + } + + err := rc.ping(test.args.ctx) + if err := test.checkFunc(test.want, err); err != nil { + tt.Errorf("error = %v", err) + } + + }) + } +} From 22271bd18055e2dfe16725c32a6887e1578d5777 Mon Sep 17 00:00:00 2001 From: hlts2 Date: Tue, 21 Jul 2020 18:09:28 +0900 Subject: [PATCH 3/6] feat: test code Signed-off-by: hlts2 --- internal/db/kvs/redis/option.go | 6 +- internal/db/kvs/redis/redis_mock_test.go | 38 ++ internal/db/kvs/redis/redis_test.go | 509 +++++++++++++++-------- 3 files changed, 379 insertions(+), 174 deletions(-) create mode 100644 internal/db/kvs/redis/redis_mock_test.go diff --git a/internal/db/kvs/redis/option.go b/internal/db/kvs/redis/option.go index 596bd91e8c..e917ef28cb 100644 --- a/internal/db/kvs/redis/option.go +++ b/internal/db/kvs/redis/option.go @@ -33,7 +33,7 @@ var ( defaultOpts = []Option{ WithInitialPingDuration("30ms"), WithInitialPingTimeLimit("5m"), - WithPingFlag(true), + WithPing(true), } ) @@ -324,9 +324,9 @@ func WithInitialPingDuration(dur string) Option { } } -func WithPingFlag(flag bool) Option { +func WithPing(enabled bool) Option { return func(r *redisClient) error { - r.pingEnabled = flag + r.pingEnabled = enabled return nil } } diff --git a/internal/db/kvs/redis/redis_mock_test.go b/internal/db/kvs/redis/redis_mock_test.go new file mode 100644 index 0000000000..e22cd9547c --- /dev/null +++ b/internal/db/kvs/redis/redis_mock_test.go @@ -0,0 +1,38 @@ +package redis + +import redis "github.com/go-redis/redis/v7" + +type MockRedis struct { + TxPipelineFunc func() redis.Pipeliner + PingFunc func() *StatusCmd + CloseFunc func() error + GetFunc func(string) *redis.StringCmd + MGetFunc func(...string) *redis.SliceCmd + DelFunc func(keys ...string) *redis.IntCmd +} + +var _ = (*MockRedis)(nil) + +func (m *MockRedis) TxPipeline() redis.Pipeliner { + return m.TxPipelineFunc() +} + +func (m *MockRedis) Ping() *StatusCmd { + return m.PingFunc() +} + +func (m *MockRedis) Close() error { + return m.CloseFunc() +} + +func (m *MockRedis) Get(key string) *redis.StringCmd { + return m.GetFunc(key) +} + +func (m *MockRedis) MGet(keys ...string) *redis.SliceCmd { + return m.MGetFunc(keys...) +} + +func (m *MockRedis) Del(keys ...string) *redis.IntCmd { + return m.DelFunc(keys...) +} diff --git a/internal/db/kvs/redis/redis_test.go b/internal/db/kvs/redis/redis_test.go index a76690aee0..234df92ab0 100644 --- a/internal/db/kvs/redis/redis_test.go +++ b/internal/db/kvs/redis/redis_test.go @@ -19,16 +19,36 @@ package redis import ( "context" "crypto/tls" + "fmt" + "os" "reflect" "testing" "time" - redis "github.com/go-redis/redis/v7" + "github.com/go-redis/redis/v7" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/vdaas/vald/internal/errors" + "github.com/vdaas/vald/internal/log" "github.com/vdaas/vald/internal/net" "go.uber.org/goleak" ) +var ( + // Goroutine leak is detected by `fastime`, but it should be ignored in the test because it is an external package. + goleakIgnoreOptions = []goleak.Option{ + goleak.IgnoreTopFunction("github.com/kpango/fastime.(*Fastime).StartTimerD.func1"), + goleak.IgnoreTopFunction("github.com/go-redis/redis/v7/internal/pool.(*ConnPool).reaper"), + goleak.IgnoreTopFunction("github.com/go-redis/redis/v7.(*ClusterClient).reaper"), + } +) + +func TestMain(m *testing.M) { + log.Init() + code := m.Run() + os.Exit(code) +} + func TestNew(t *testing.T) { type args struct { ctx context.Context @@ -50,44 +70,284 @@ func TestNew(t *testing.T) { if !errors.Is(err, w.err) { return errors.Errorf("got error = %v, want %v", err, w.err) } - if !reflect.DeepEqual(gotRc, w.wantRc) { - return errors.Errorf("got = %v, want %v", gotRc, w.wantRc) + if !reflect.DeepEqual(w.wantRc, gotRc) { + return errors.Errorf("got = %v, want = %v", gotRc, w.wantRc) } + return nil } + tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - ctx: nil, - opts: nil, - }, - want: want{}, - checkFunc: defaultCheckFunc, - }, - */ + func() test { + dialer := func(ctx context.Context, addr, port string) (net.Conn, error) { + return nil, nil + } + connFn := func(*redis.Conn) error { + return nil + } + cfg := new(tls.Config) + + return test{ + name: "returns Redis implementation when address length is 1", + args: args{ + ctx: context.Background(), + opts: []Option{ + WithAddrs("127.0.0.1"), + WithPassword("pass"), + WithDialer(dialer), + WithOnConnectFunction(connFn), + WithDB(1), + WithRetryLimit(2), + WithMinimumRetryBackoff("3s"), + WithMaximumRetryBackoff("4s"), + WithDialTimeout("5s"), + WithReadTimeout("6s"), + WithWriteTimeout("7s"), + WithPoolSize(8), + WithMinimumIdleConnection(9), + WithMaximumConnectionAge("10s"), + WithPoolTimeout("11s"), + WithIdleTimeout("12s"), + WithIdleCheckFrequency("13s"), + WithTLSConfig(cfg), + WithPing(false), + }, + }, + want: want{ + wantRc: redis.NewClient(&redis.Options{ + Addr: "127.0.0.1", + Password: "pass", + Dialer: dialer, + OnConnect: connFn, + DB: 1, + MaxRetries: 2, + MinRetryBackoff: 3 * time.Second, + MaxRetryBackoff: 4 * time.Second, + DialTimeout: 5 * time.Second, + ReadTimeout: 6 * time.Second, + WriteTimeout: 7 * time.Second, + PoolSize: 8, + MinIdleConns: 9, + MaxConnAge: 10 * time.Second, + PoolTimeout: 11 * time.Second, + IdleTimeout: 12 * time.Second, + IdleCheckFrequency: 13 * time.Second, + TLSConfig: cfg, + }), + err: nil, + }, + checkFunc: func(w want, gotRc Redis, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if gotRc == nil { + return errors.New("got is nil") + } + + var ( + want = w.wantRc.(*redis.Client).Options() + got = gotRc.(*redis.Client).Options() + ) + + opts := []cmp.Option{ + cmpopts.IgnoreUnexported(*want), + cmpopts.IgnoreUnexported(*got), + cmp.Comparer(func(want, got *tls.Config) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + cmp.Comparer(func(want, got func(ctx context.Context, network, addr string) (net.Conn, error)) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + cmp.Comparer(func(want, got func(*redis.Conn) error) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + } + if diff := cmp.Diff(want, got, opts...); diff != "" { + return errors.Errorf("got = %v, want = %v", got, want) + } + + return nil + }, + } + }(), + + func() test { + dialer := func(ctx context.Context, addr, port string) (net.Conn, error) { + return nil, nil + } + closterSlots := func() ([]redis.ClusterSlot, error) { + return nil, nil + } + onNewNode := func(*redis.Client) {} + onConnect := func(*redis.Conn) error { + return nil + } + cfg := new(tls.Config) + + return test{ + name: "returns Redis implementation when address length is 2", + args: args{ + ctx: context.Background(), + opts: []Option{ + WithAddrs("127.0.0.1", "192.168.33.10"), + WithDialer(dialer), + WithRedirectLimit(1), + WithReadOnlyFlag(true), + WithRouteByLatencyFlag(true), + WithRouteRandomlyFlag(true), + WithClusterSlots(closterSlots), + WithOnNewNodeFunction(onNewNode), + WithOnConnectFunction(onConnect), + WithPassword("pass"), + WithRetryLimit(2), + WithMinimumRetryBackoff("3s"), + WithMaximumRetryBackoff("4s"), + WithDialTimeout("5s"), + WithReadTimeout("6s"), + WithWriteTimeout("7s"), + WithPoolSize(8), + WithMinimumIdleConnection(9), + WithMaximumConnectionAge("10s"), + WithPoolTimeout("11s"), + WithIdleTimeout("12s"), + WithIdleCheckFrequency("13s"), + WithTLSConfig(cfg), + WithPing(false), + }, + }, + want: want{ + wantRc: redis.NewClusterClient(&redis.ClusterOptions{ + Addrs: []string{ + "127.0.0.1", "192.168.33.10", + }, + Dialer: dialer, + MaxRedirects: 1, + ReadOnly: true, + RouteByLatency: true, + RouteRandomly: true, + ClusterSlots: closterSlots, + OnNewNode: onNewNode, + OnConnect: onConnect, + Password: "pass", + MaxRetries: 2, + MinRetryBackoff: 3 * time.Second, + MaxRetryBackoff: 4 * time.Second, + DialTimeout: 5 * time.Second, + ReadTimeout: 6 * time.Second, + WriteTimeout: 7 * time.Second, + PoolSize: 8, + MinIdleConns: 9, + MaxConnAge: 10 * time.Second, + PoolTimeout: 11 * time.Second, + IdleTimeout: 12 * time.Second, + IdleCheckFrequency: 13 * time.Second, + TLSConfig: cfg, + }), + err: nil, + }, + checkFunc: func(w want, gotRc Redis, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if gotRc == nil { + return errors.New("got is nil") + } + + var ( + want = w.wantRc.(*redis.ClusterClient).Options() + got = gotRc.(*redis.ClusterClient).Options() + ) + + opts := []cmp.Option{ + cmpopts.IgnoreUnexported(*want), + cmpopts.IgnoreUnexported(*got), + cmp.Comparer(func(want, got func(opt *redis.Options) *redis.Client) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + cmp.Comparer(func(want, got func(*redis.Client)) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + cmp.Comparer(func(want, got func() ([]redis.ClusterSlot, error)) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + cmp.Comparer(func(want, got func(ctx context.Context, network, addr string) (net.Conn, error)) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + cmp.Comparer(func(want, got func(*redis.Conn) error) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + cmp.Comparer(func(want, got *tls.Config) bool { + return reflect.ValueOf(want).Pointer() == reflect.ValueOf(got).Pointer() + }), + } + if diff := cmp.Diff(want, got, opts...); diff != "" { + fmt.Println(diff) + return errors.Errorf("got = %v, want = %v", got, want) + } + + return nil + }, + } + }(), + + func() test { + return test{ + name: "returns redis address not found error when address length is 0", + args: args{ + ctx: context.Background(), + opts: nil, + }, + want: want{ + wantRc: nil, + err: errors.ErrRedisAddrsNotFound, + }, + } + }(), + + func() test { + return test{ + name: "returns redis address not found error when address length is 1 but address contains empty string", + args: args{ + ctx: context.Background(), + opts: []Option{ + WithAddrs(""), + }, + }, + want: want{ + wantRc: nil, + err: errors.ErrRedisAddrsNotFound, + }, + } + }(), - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - ctx: nil, - opts: nil, - }, - want: want{}, - checkFunc: defaultCheckFunc, - } - }(), - */ + func() test { + err := errors.New("err") + return test{ + name: "returns ping error when address length is 1 and ping fails", + args: args{ + ctx: context.Background(), + opts: []Option{ + WithAddrs("127.0.0.01"), + WithInitialPingDuration("1ms"), + WithInitialPingTimeLimit("2ms"), + WithDialer(func(ctx context.Context, addr string, port string) (net.Conn, error) { + return nil, err + }), + }, + }, + want: want{ + wantRc: nil, + err: errors.Wrap(errors.Wrap( + err, + errors.ErrRedisConnectionPingFailed.Error()), context.DeadlineExceeded.Error()), + }, + } + }(), } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(t) + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } @@ -102,7 +362,6 @@ func TestNew(t *testing.T) { if err := test.checkFunc(test.want, gotRc, err); err != nil { tt.Errorf("error = %v", err) } - }) } } @@ -112,35 +371,9 @@ func Test_redisClient_ping(t *testing.T) { ctx context.Context } type fields struct { - addrs []string - clusterSlots func() ([]redis.ClusterSlot, error) - db int - dialTimeout time.Duration - dialer func(ctx context.Context, network, addr string) (net.Conn, error) - idleCheckFrequency time.Duration - idleTimeout time.Duration initialPingDuration time.Duration initialPingTimeLimit time.Duration - keyPref string - maxConnAge time.Duration - maxRedirects int - maxRetries int - maxRetryBackoff time.Duration - minIdleConns int - minRetryBackoff time.Duration - onConnect func(*redis.Conn) error - onNewNode func(*redis.Client) - password string - poolSize int - poolTimeout time.Duration - readOnly bool - readTimeout time.Duration - routeByLatency bool - routeRandomly bool - tlsConfig *tls.Config - writeTimeout time.Duration client Redis - pingEnabled bool } type want struct { err error @@ -161,98 +394,59 @@ func Test_redisClient_ping(t *testing.T) { return nil } tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - ctx: nil, - }, - fields: fields { - addrs: nil, - clusterSlots: nil, - db: 0, - dialTimeout: nil, - dialer: nil, - idleCheckFrequency: nil, - idleTimeout: nil, - initialPingDuration: nil, - initialPingTimeLimit: nil, - keyPref: "", - maxConnAge: nil, - maxRedirects: 0, - maxRetries: 0, - maxRetryBackoff: nil, - minIdleConns: 0, - minRetryBackoff: nil, - onConnect: nil, - onNewNode: nil, - password: "", - poolSize: 0, - poolTimeout: nil, - readOnly: false, - readTimeout: nil, - routeByLatency: false, - routeRandomly: false, - tlsConfig: nil, - writeTimeout: nil, - client: nil, - pingEnabled: false, - }, - want: want{}, - checkFunc: defaultCheckFunc, - }, - */ + func() test { + return test{ + name: "returns nil when the ping success", + args: args{ + ctx: context.Background(), + }, + fields: fields{ + initialPingDuration: time.Microsecond, + initialPingTimeLimit: time.Second, + client: func() Redis { + return &MockRedis{ + PingFunc: func() *StatusCmd { + return new(StatusCmd) + }, + } + }(), + }, + want: want{ + err: nil, + }, + } + }(), - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - ctx: nil, - }, - fields: fields { - addrs: nil, - clusterSlots: nil, - db: 0, - dialTimeout: nil, - dialer: nil, - idleCheckFrequency: nil, - idleTimeout: nil, - initialPingDuration: nil, - initialPingTimeLimit: nil, - keyPref: "", - maxConnAge: nil, - maxRedirects: 0, - maxRetries: 0, - maxRetryBackoff: nil, - minIdleConns: 0, - minRetryBackoff: nil, - onConnect: nil, - onNewNode: nil, - password: "", - poolSize: 0, - poolTimeout: nil, - readOnly: false, - readTimeout: nil, - routeByLatency: false, - routeRandomly: false, - tlsConfig: nil, - writeTimeout: nil, - client: nil, - pingEnabled: false, - }, - want: want{}, - checkFunc: defaultCheckFunc, - } - }(), - */ + func() test { + err := errors.New("err") + return test{ + name: "returns ping failed error when the ping fails and reached the ping time limit", + args: args{ + ctx: context.Background(), + }, + fields: fields{ + initialPingDuration: time.Millisecond, + initialPingTimeLimit: 5 * time.Millisecond, + client: func() Redis { + return &MockRedis{ + PingFunc: func() (cmd *StatusCmd) { + cmd = new(StatusCmd) + cmd.SetErr(err) + return + }, + } + }(), + }, + want: want{ + err: errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), context.DeadlineExceeded.Error()), + }, + } + }(), } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(tt) + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } @@ -263,42 +457,15 @@ func Test_redisClient_ping(t *testing.T) { test.checkFunc = defaultCheckFunc } rc := &redisClient{ - addrs: test.fields.addrs, - clusterSlots: test.fields.clusterSlots, - db: test.fields.db, - dialTimeout: test.fields.dialTimeout, - dialer: test.fields.dialer, - idleCheckFrequency: test.fields.idleCheckFrequency, - idleTimeout: test.fields.idleTimeout, initialPingDuration: test.fields.initialPingDuration, initialPingTimeLimit: test.fields.initialPingTimeLimit, - keyPref: test.fields.keyPref, - maxConnAge: test.fields.maxConnAge, - maxRedirects: test.fields.maxRedirects, - maxRetries: test.fields.maxRetries, - maxRetryBackoff: test.fields.maxRetryBackoff, - minIdleConns: test.fields.minIdleConns, - minRetryBackoff: test.fields.minRetryBackoff, - onConnect: test.fields.onConnect, - onNewNode: test.fields.onNewNode, - password: test.fields.password, - poolSize: test.fields.poolSize, - poolTimeout: test.fields.poolTimeout, - readOnly: test.fields.readOnly, - readTimeout: test.fields.readTimeout, - routeByLatency: test.fields.routeByLatency, - routeRandomly: test.fields.routeRandomly, - tlsConfig: test.fields.tlsConfig, - writeTimeout: test.fields.writeTimeout, client: test.fields.client, - pingEnabled: test.fields.pingEnabled, } err := rc.ping(test.args.ctx) if err := test.checkFunc(test.want, err); err != nil { tt.Errorf("error = %v", err) } - }) } } From a7934a43ca46b838d250f060e70dba1be5cae77f Mon Sep 17 00:00:00 2001 From: hlts2 Date: Mon, 27 Jul 2020 23:36:17 +0900 Subject: [PATCH 4/6] fix: feedback of author Signed-off-by: hlts2 --- internal/db/kvs/redis/option.go | 8 - internal/db/kvs/redis/option_test.go | 114 ------- internal/db/kvs/redis/redis.go | 117 ++++---- internal/db/kvs/redis/redis_test.go | 434 +++++++++++++++++---------- 4 files changed, 334 insertions(+), 339 deletions(-) diff --git a/internal/db/kvs/redis/option.go b/internal/db/kvs/redis/option.go index e917ef28cb..5b30a427b5 100644 --- a/internal/db/kvs/redis/option.go +++ b/internal/db/kvs/redis/option.go @@ -33,7 +33,6 @@ var ( defaultOpts = []Option{ WithInitialPingDuration("30ms"), WithInitialPingTimeLimit("5m"), - WithPing(true), } ) @@ -323,10 +322,3 @@ func WithInitialPingDuration(dur string) Option { return nil } } - -func WithPing(enabled bool) Option { - return func(r *redisClient) error { - r.pingEnabled = enabled - return nil - } -} diff --git a/internal/db/kvs/redis/option_test.go b/internal/db/kvs/redis/option_test.go index d8c009ce91..0be610e31f 100644 --- a/internal/db/kvs/redis/option_test.go +++ b/internal/db/kvs/redis/option_test.go @@ -3077,117 +3077,3 @@ func TestWithInitialPingDuration(t *testing.T) { }) } } - -func TestWithPingFlag(t *testing.T) { - // Change interface type to the type of object you are testing - type T = interface{} - type args struct { - flag bool - } - type want struct { - obj *T - // Uncomment this line if the option returns an error, otherwise delete it - // err error - } - type test struct { - name string - args args - want want - // Use the first line if the option returns an error. otherwise use the second line - // checkFunc func(want, *T, error) error - // checkFunc func(want, *T) error - beforeFunc func(args) - afterFunc func(args) - } - - // Uncomment this block if the option returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T, err error) error { - if !errors.Is(err, w.err) { - return errors.Errorf("got error = %v, want %v", err, w.err) - } - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.obj) - } - return nil - } - */ - - // Uncomment this block if the option do not returns an error, otherwise delete it - /* - defaultCheckFunc := func(w want, obj *T) error { - if !reflect.DeepEqual(obj, w.obj) { - return errors.Errorf("got = %v, want %v", obj, w.obj) - } - return nil - } - */ - - tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - flag: false, - }, - want: want { - obj: new(T), - }, - }, - */ - - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - flag: false, - }, - want: want { - obj: new(T), - }, - } - }(), - */ - } - - for _, test := range tests { - t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(tt) - if test.beforeFunc != nil { - test.beforeFunc(test.args) - } - if test.afterFunc != nil { - defer test.afterFunc(test.args) - } - - // Uncomment this block if the option returns an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - - got := WithPingFlag(test.args.flag) - obj := new(T) - if err := test.checkFunc(test.want, obj, got(obj)); err != nil { - tt.Errorf("error = %v", err) - } - */ - - // Uncomment this block if the option do not return an error, otherwise delete it - /* - if test.checkFunc == nil { - test.checkFunc = defaultCheckFunc - } - got := WithPingFlag(test.args.flag) - obj := new(T) - got(obj) - if err := test.checkFunc(test.want, obj); err != nil { - tt.Errorf("error = %v", err) - } - */ - }) - } -} diff --git a/internal/db/kvs/redis/redis.go b/internal/db/kvs/redis/redis.go index 44a5e25463..3828b92455 100644 --- a/internal/db/kvs/redis/redis.go +++ b/internal/db/kvs/redis/redis.go @@ -83,9 +83,7 @@ type redisClient struct { routeRandomly bool tlsConfig *tls.Config writeTimeout time.Duration - - client Redis - pingEnabled bool + client Redis } // New returns Redis implementation if no error occurs. @@ -97,82 +95,85 @@ func New(ctx context.Context, opts ...Option) (rc Redis, err error) { } } - switch len(r.addrs) { + r, err = r.newRedisClient(ctx) + if err != nil { + return nil, err + } + + return r.ping(ctx) +} + +func (rc *redisClient) newRedisClient(ctx context.Context) (*redisClient, error) { + switch len(rc.addrs) { case 0: return nil, errors.ErrRedisAddrsNotFound case 1: - if len(r.addrs[0]) == 0 { + if len(rc.addrs[0]) == 0 { return nil, errors.ErrRedisAddrsNotFound } - r.client = redis.NewClient(&redis.Options{ - Addr: r.addrs[0], - Password: r.password, - Dialer: r.dialer, - OnConnect: r.onConnect, - DB: r.db, - MaxRetries: r.maxRetries, - MinRetryBackoff: r.minRetryBackoff, - MaxRetryBackoff: r.maxRetryBackoff, - DialTimeout: r.dialTimeout, - ReadTimeout: r.readTimeout, - WriteTimeout: r.writeTimeout, - PoolSize: r.poolSize, - MinIdleConns: r.minIdleConns, - MaxConnAge: r.maxConnAge, - PoolTimeout: r.poolTimeout, - IdleTimeout: r.idleTimeout, - IdleCheckFrequency: r.idleCheckFrequency, - TLSConfig: r.tlsConfig, + rc.client = redis.NewClient(&redis.Options{ + Addr: rc.addrs[0], + Password: rc.password, + Dialer: rc.dialer, + OnConnect: rc.onConnect, + DB: rc.db, + MaxRetries: rc.maxRetries, + MinRetryBackoff: rc.minRetryBackoff, + MaxRetryBackoff: rc.maxRetryBackoff, + DialTimeout: rc.dialTimeout, + ReadTimeout: rc.readTimeout, + WriteTimeout: rc.writeTimeout, + PoolSize: rc.poolSize, + MinIdleConns: rc.minIdleConns, + MaxConnAge: rc.maxConnAge, + PoolTimeout: rc.poolTimeout, + IdleTimeout: rc.idleTimeout, + IdleCheckFrequency: rc.idleCheckFrequency, + TLSConfig: rc.tlsConfig, }) default: - r.client = redis.NewClusterClient(&redis.ClusterOptions{ - Addrs: r.addrs, - Dialer: r.dialer, - MaxRedirects: r.maxRedirects, - ReadOnly: r.readOnly, - RouteByLatency: r.routeByLatency, - RouteRandomly: r.routeRandomly, - ClusterSlots: r.clusterSlots, - OnNewNode: r.onNewNode, - OnConnect: r.onConnect, - Password: r.password, - MaxRetries: r.maxRetries, - MinRetryBackoff: r.minRetryBackoff, - MaxRetryBackoff: r.maxRetryBackoff, - DialTimeout: r.dialTimeout, - ReadTimeout: r.readTimeout, - WriteTimeout: r.writeTimeout, - PoolSize: r.poolSize, - MinIdleConns: r.minIdleConns, - MaxConnAge: r.maxConnAge, - PoolTimeout: r.poolTimeout, - IdleTimeout: r.idleTimeout, - IdleCheckFrequency: r.idleCheckFrequency, - TLSConfig: r.tlsConfig, + rc.client = redis.NewClusterClient(&redis.ClusterOptions{ + Addrs: rc.addrs, + Dialer: rc.dialer, + MaxRedirects: rc.maxRedirects, + ReadOnly: rc.readOnly, + RouteByLatency: rc.routeByLatency, + RouteRandomly: rc.routeRandomly, + ClusterSlots: rc.clusterSlots, + OnNewNode: rc.onNewNode, + OnConnect: rc.onConnect, + Password: rc.password, + MaxRetries: rc.maxRetries, + MinRetryBackoff: rc.minRetryBackoff, + MaxRetryBackoff: rc.maxRetryBackoff, + DialTimeout: rc.dialTimeout, + ReadTimeout: rc.readTimeout, + WriteTimeout: rc.writeTimeout, + PoolSize: rc.poolSize, + MinIdleConns: rc.minIdleConns, + MaxConnAge: rc.maxConnAge, + PoolTimeout: rc.poolTimeout, + IdleTimeout: rc.idleTimeout, + IdleCheckFrequency: rc.idleCheckFrequency, + TLSConfig: rc.tlsConfig, }).WithContext(ctx) } - if r.pingEnabled { - if err = r.ping(ctx); err != nil { - return nil, err - } - } - - return r.client, nil + return rc, nil } -func (rc *redisClient) ping(ctx context.Context) (err error) { +func (rc *redisClient) ping(ctx context.Context) (r Redis, err error) { pctx, cancel := context.WithTimeout(ctx, rc.initialPingTimeLimit) defer cancel() tick := time.NewTicker(rc.initialPingDuration) for { select { case <-pctx.Done(): - return errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), pctx.Err().Error()) + return nil, errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), pctx.Err().Error()) case <-tick.C: err = rc.client.Ping().Err() if err == nil { - return nil + return rc.client, nil } log.Error(err) } diff --git a/internal/db/kvs/redis/redis_test.go b/internal/db/kvs/redis/redis_test.go index 234df92ab0..1504f76aa0 100644 --- a/internal/db/kvs/redis/redis_test.go +++ b/internal/db/kvs/redis/redis_test.go @@ -25,7 +25,7 @@ import ( "testing" "time" - "github.com/go-redis/redis/v7" + redis "github.com/go-redis/redis/v7" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/vdaas/vald/internal/errors" @@ -77,12 +77,117 @@ func TestNew(t *testing.T) { return nil } + tests := []test{ + { + name: "returns address not found error when options is nil", + args: args{ + ctx: context.Background(), + }, + want: want{ + wantRc: nil, + err: errors.ErrRedisAddrsNotFound, + }, + }, + + { + name: "returns ping failed error when options is not nil", + args: args{ + ctx: context.Background(), + opts: []Option{ + WithAddrs("127.0.0.0.1"), + WithInitialPingTimeLimit("1µs"), + WithInitialPingDuration("10ms"), + }, + }, + want: want{ + wantRc: nil, + err: errors.Wrap(errors.Wrap(nil, errors.ErrRedisConnectionPingFailed.Error()), context.DeadlineExceeded.Error()), + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(tt *testing.T) { + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) + if test.beforeFunc != nil { + test.beforeFunc(test.args) + } + if test.afterFunc != nil { + defer test.afterFunc(test.args) + } + if test.checkFunc == nil { + test.checkFunc = defaultCheckFunc + } + + gotRc, err := New(test.args.ctx, test.args.opts...) + if err := test.checkFunc(test.want, gotRc, err); err != nil { + tt.Errorf("error = %v", err) + } + }) + } +} + +func Test_redisClient_newRedisClient(t *testing.T) { + type args struct { + ctx context.Context + } + type fields struct { + addrs []string + clusterSlots func() ([]redis.ClusterSlot, error) + db int + dialTimeout time.Duration + dialer func(ctx context.Context, network, addr string) (net.Conn, error) + idleCheckFrequency time.Duration + idleTimeout time.Duration + initialPingDuration time.Duration + initialPingTimeLimit time.Duration + keyPref string + maxConnAge time.Duration + maxRedirects int + maxRetries int + maxRetryBackoff time.Duration + minIdleConns int + minRetryBackoff time.Duration + onConnect func(*redis.Conn) error + onNewNode func(*redis.Client) + password string + poolSize int + poolTimeout time.Duration + readOnly bool + readTimeout time.Duration + routeByLatency bool + routeRandomly bool + tlsConfig *tls.Config + writeTimeout time.Duration + } + type want struct { + want *redisClient + err error + } + type test struct { + name string + args args + fields fields + want want + checkFunc func(want, *redisClient, error) error + beforeFunc func(args) + afterFunc func(args) + } + defaultCheckFunc := func(w want, got *redisClient, err error) error { + if !errors.Is(err, w.err) { + return errors.Errorf("got error = %v, want %v", err, w.err) + } + if !reflect.DeepEqual(got, w.want) { + return errors.Errorf("got = %v, want %v", got, w.want) + } + return nil + } tests := []test{ func() test { - dialer := func(ctx context.Context, addr, port string) (net.Conn, error) { + dialer := func(ctx context.Context, _, _ string) (net.Conn, error) { return nil, nil } - connFn := func(*redis.Conn) error { + connFn := func(c *redis.Conn) error { return nil } cfg := new(tls.Config) @@ -91,52 +196,53 @@ func TestNew(t *testing.T) { name: "returns Redis implementation when address length is 1", args: args{ ctx: context.Background(), - opts: []Option{ - WithAddrs("127.0.0.1"), - WithPassword("pass"), - WithDialer(dialer), - WithOnConnectFunction(connFn), - WithDB(1), - WithRetryLimit(2), - WithMinimumRetryBackoff("3s"), - WithMaximumRetryBackoff("4s"), - WithDialTimeout("5s"), - WithReadTimeout("6s"), - WithWriteTimeout("7s"), - WithPoolSize(8), - WithMinimumIdleConnection(9), - WithMaximumConnectionAge("10s"), - WithPoolTimeout("11s"), - WithIdleTimeout("12s"), - WithIdleCheckFrequency("13s"), - WithTLSConfig(cfg), - WithPing(false), - }, + }, + fields: fields{ + addrs: []string{"127.0.0.1"}, + password: "pass", + dialer: dialer, + onConnect: connFn, + db: 1, + maxRetries: 2, + minRetryBackoff: 3 * time.Second, + maxRetryBackoff: 4 * time.Second, + dialTimeout: 5 * time.Second, + readTimeout: 6 * time.Second, + writeTimeout: 7 * time.Second, + poolSize: 8, + minIdleConns: 9, + maxConnAge: 10 * time.Second, + poolTimeout: 11 * time.Second, + idleTimeout: 12 * time.Second, + idleCheckFrequency: 13 * time.Second, + tlsConfig: cfg, }, want: want{ - wantRc: redis.NewClient(&redis.Options{ - Addr: "127.0.0.1", - Password: "pass", - Dialer: dialer, - OnConnect: connFn, - DB: 1, - MaxRetries: 2, - MinRetryBackoff: 3 * time.Second, - MaxRetryBackoff: 4 * time.Second, - DialTimeout: 5 * time.Second, - ReadTimeout: 6 * time.Second, - WriteTimeout: 7 * time.Second, - PoolSize: 8, - MinIdleConns: 9, - MaxConnAge: 10 * time.Second, - PoolTimeout: 11 * time.Second, - IdleTimeout: 12 * time.Second, - IdleCheckFrequency: 13 * time.Second, - TLSConfig: cfg, - }), + want: &redisClient{ + client: redis.NewClient(&redis.Options{ + Addr: "127.0.0.1", + Password: "pass", + Dialer: dialer, + OnConnect: connFn, + DB: 1, + MaxRetries: 2, + MinRetryBackoff: 3 * time.Second, + MaxRetryBackoff: 4 * time.Second, + DialTimeout: 5 * time.Second, + ReadTimeout: 6 * time.Second, + WriteTimeout: 7 * time.Second, + PoolSize: 8, + MinIdleConns: 9, + MaxConnAge: 10 * time.Second, + PoolTimeout: 11 * time.Second, + IdleTimeout: 12 * time.Second, + IdleCheckFrequency: 13 * time.Second, + TLSConfig: cfg, + }), + }, err: nil, }, - checkFunc: func(w want, gotRc Redis, err error) error { + checkFunc: func(w want, gotRc *redisClient, err error) error { if !errors.Is(err, w.err) { return errors.Errorf("got error = %v, want %v", err, w.err) } @@ -145,8 +251,8 @@ func TestNew(t *testing.T) { } var ( - want = w.wantRc.(*redis.Client).Options() - got = gotRc.(*redis.Client).Options() + want = w.want.client.(*redis.Client).Options() + got = gotRc.client.(*redis.Client).Options() ) opts := []cmp.Option{ @@ -163,7 +269,7 @@ func TestNew(t *testing.T) { }), } if diff := cmp.Diff(want, got, opts...); diff != "" { - return errors.Errorf("got = %v, want = %v", got, want) + return errors.Errorf("client options diff: %s", diff) } return nil @@ -172,14 +278,14 @@ func TestNew(t *testing.T) { }(), func() test { - dialer := func(ctx context.Context, addr, port string) (net.Conn, error) { + dialer := func(ctx context.Context, _, _ string) (net.Conn, error) { return nil, nil } - closterSlots := func() ([]redis.ClusterSlot, error) { + cslots := func() ([]redis.ClusterSlot, error) { return nil, nil } onNewNode := func(*redis.Client) {} - onConnect := func(*redis.Conn) error { + onConnect := func(c *redis.Conn) error { return nil } cfg := new(tls.Config) @@ -188,64 +294,61 @@ func TestNew(t *testing.T) { name: "returns Redis implementation when address length is 2", args: args{ ctx: context.Background(), - opts: []Option{ - WithAddrs("127.0.0.1", "192.168.33.10"), - WithDialer(dialer), - WithRedirectLimit(1), - WithReadOnlyFlag(true), - WithRouteByLatencyFlag(true), - WithRouteRandomlyFlag(true), - WithClusterSlots(closterSlots), - WithOnNewNodeFunction(onNewNode), - WithOnConnectFunction(onConnect), - WithPassword("pass"), - WithRetryLimit(2), - WithMinimumRetryBackoff("3s"), - WithMaximumRetryBackoff("4s"), - WithDialTimeout("5s"), - WithReadTimeout("6s"), - WithWriteTimeout("7s"), - WithPoolSize(8), - WithMinimumIdleConnection(9), - WithMaximumConnectionAge("10s"), - WithPoolTimeout("11s"), - WithIdleTimeout("12s"), - WithIdleCheckFrequency("13s"), - WithTLSConfig(cfg), - WithPing(false), - }, + }, + fields: fields{ + addrs: []string{"127.0.0.1", "127.0.0.2"}, + dialer: dialer, + maxRedirects: 1, + readOnly: true, + routeByLatency: true, + routeRandomly: true, + clusterSlots: cslots, + onNewNode: onNewNode, + onConnect: onConnect, + password: "pass", + maxRetries: 2, + minRetryBackoff: 3 * time.Second, + maxRetryBackoff: 4 * time.Second, + dialTimeout: 5 * time.Second, + readTimeout: 6 * time.Second, + writeTimeout: 7 * time.Second, + poolSize: 8, + maxConnAge: 9 * time.Second, + idleTimeout: 10 * time.Second, + idleCheckFrequency: 11 * time.Second, + tlsConfig: cfg, }, want: want{ - wantRc: redis.NewClusterClient(&redis.ClusterOptions{ - Addrs: []string{ - "127.0.0.1", "192.168.33.10", - }, - Dialer: dialer, - MaxRedirects: 1, - ReadOnly: true, - RouteByLatency: true, - RouteRandomly: true, - ClusterSlots: closterSlots, - OnNewNode: onNewNode, - OnConnect: onConnect, - Password: "pass", - MaxRetries: 2, - MinRetryBackoff: 3 * time.Second, - MaxRetryBackoff: 4 * time.Second, - DialTimeout: 5 * time.Second, - ReadTimeout: 6 * time.Second, - WriteTimeout: 7 * time.Second, - PoolSize: 8, - MinIdleConns: 9, - MaxConnAge: 10 * time.Second, - PoolTimeout: 11 * time.Second, - IdleTimeout: 12 * time.Second, - IdleCheckFrequency: 13 * time.Second, - TLSConfig: cfg, - }), + want: &redisClient{ + client: redis.NewClusterClient(&redis.ClusterOptions{ + Addrs: []string{ + "127.0.0.1", "127.0.0.2", + }, + Dialer: dialer, + MaxRedirects: 1, + ReadOnly: true, + RouteByLatency: true, + RouteRandomly: true, + ClusterSlots: cslots, + OnNewNode: onNewNode, + OnConnect: onConnect, + Password: "pass", + MaxRetries: 2, + MinRetryBackoff: 3 * time.Second, + MaxRetryBackoff: 4 * time.Second, + DialTimeout: 5 * time.Second, + ReadTimeout: 6 * time.Second, + WriteTimeout: 7 * time.Second, + PoolSize: 8, + MaxConnAge: 9 * time.Second, + IdleTimeout: 10 * time.Second, + IdleCheckFrequency: 11 * time.Second, + TLSConfig: cfg, + }), + }, err: nil, }, - checkFunc: func(w want, gotRc Redis, err error) error { + checkFunc: func(w want, gotRc *redisClient, err error) error { if !errors.Is(err, w.err) { return errors.Errorf("got error = %v, want %v", err, w.err) } @@ -254,8 +357,8 @@ func TestNew(t *testing.T) { } var ( - want = w.wantRc.(*redis.ClusterClient).Options() - got = gotRc.(*redis.ClusterClient).Options() + want = w.want.client.(*redis.ClusterClient).Options() + got = gotRc.client.(*redis.ClusterClient).Options() ) opts := []cmp.Option{ @@ -292,54 +395,29 @@ func TestNew(t *testing.T) { func() test { return test{ - name: "returns redis address not found error when address length is 0", - args: args{ - ctx: context.Background(), - opts: nil, - }, - want: want{ - wantRc: nil, - err: errors.ErrRedisAddrsNotFound, - }, - } - }(), - - func() test { - return test{ - name: "returns redis address not found error when address length is 1 but address contains empty string", + name: "returns address not found error when address length is 0", args: args{ ctx: context.Background(), - opts: []Option{ - WithAddrs(""), - }, }, want: want{ - wantRc: nil, - err: errors.ErrRedisAddrsNotFound, + want: nil, + err: errors.ErrRedisAddrsNotFound, }, } }(), func() test { - err := errors.New("err") return test{ - name: "returns ping error when address length is 1 and ping fails", + name: "returns address not found error when address length is 1 but contains empty string", + fields: fields{ + addrs: []string{""}, + }, args: args{ ctx: context.Background(), - opts: []Option{ - WithAddrs("127.0.0.01"), - WithInitialPingDuration("1ms"), - WithInitialPingTimeLimit("2ms"), - WithDialer(func(ctx context.Context, addr string, port string) (net.Conn, error) { - return nil, err - }), - }, }, want: want{ - wantRc: nil, - err: errors.Wrap(errors.Wrap( - err, - errors.ErrRedisConnectionPingFailed.Error()), context.DeadlineExceeded.Error()), + want: nil, + err: errors.ErrRedisAddrsNotFound, }, } }(), @@ -357,11 +435,41 @@ func TestNew(t *testing.T) { if test.checkFunc == nil { test.checkFunc = defaultCheckFunc } + rc := &redisClient{ + addrs: test.fields.addrs, + clusterSlots: test.fields.clusterSlots, + db: test.fields.db, + dialTimeout: test.fields.dialTimeout, + dialer: test.fields.dialer, + idleCheckFrequency: test.fields.idleCheckFrequency, + idleTimeout: test.fields.idleTimeout, + initialPingDuration: test.fields.initialPingDuration, + initialPingTimeLimit: test.fields.initialPingTimeLimit, + keyPref: test.fields.keyPref, + maxConnAge: test.fields.maxConnAge, + maxRedirects: test.fields.maxRedirects, + maxRetries: test.fields.maxRetries, + maxRetryBackoff: test.fields.maxRetryBackoff, + minIdleConns: test.fields.minIdleConns, + minRetryBackoff: test.fields.minRetryBackoff, + onConnect: test.fields.onConnect, + onNewNode: test.fields.onNewNode, + password: test.fields.password, + poolSize: test.fields.poolSize, + poolTimeout: test.fields.poolTimeout, + readOnly: test.fields.readOnly, + readTimeout: test.fields.readTimeout, + routeByLatency: test.fields.routeByLatency, + routeRandomly: test.fields.routeRandomly, + tlsConfig: test.fields.tlsConfig, + writeTimeout: test.fields.writeTimeout, + } - gotRc, err := New(test.args.ctx, test.args.opts...) - if err := test.checkFunc(test.want, gotRc, err); err != nil { + got, err := rc.newRedisClient(test.args.ctx) + if err := test.checkFunc(test.want, got, err); err != nil { tt.Errorf("error = %v", err) } + }) } } @@ -376,49 +484,55 @@ func Test_redisClient_ping(t *testing.T) { client Redis } type want struct { - err error + wantR Redis + err error } type test struct { name string args args fields fields want want - checkFunc func(want, error) error + checkFunc func(want, Redis, error) error beforeFunc func(args) afterFunc func(args) } - defaultCheckFunc := func(w want, err error) error { + defaultCheckFunc := func(w want, gotR Redis, err error) error { if !errors.Is(err, w.err) { return errors.Errorf("got error = %v, want %v", err, w.err) } + if !reflect.DeepEqual(gotR, w.wantR) { + return errors.Errorf("got = %v, want %v", gotR, w.wantR) + } return nil } tests := []test{ func() test { + r := &MockRedis{ + PingFunc: func() *StatusCmd { + return new(StatusCmd) + }, + } + return test{ name: "returns nil when the ping success", args: args{ ctx: context.Background(), }, fields: fields{ - initialPingDuration: time.Microsecond, + initialPingDuration: time.Millisecond, initialPingTimeLimit: time.Second, - client: func() Redis { - return &MockRedis{ - PingFunc: func() *StatusCmd { - return new(StatusCmd) - }, - } - }(), + client: r, }, want: want{ - err: nil, + wantR: r, + err: nil, }, } }(), func() test { err := errors.New("err") + return test{ name: "returns ping failed error when the ping fails and reached the ping time limit", args: args{ @@ -426,7 +540,7 @@ func Test_redisClient_ping(t *testing.T) { }, fields: fields{ initialPingDuration: time.Millisecond, - initialPingTimeLimit: 5 * time.Millisecond, + initialPingTimeLimit: 3 * time.Millisecond, client: func() Redis { return &MockRedis{ PingFunc: func() (cmd *StatusCmd) { @@ -438,7 +552,8 @@ func Test_redisClient_ping(t *testing.T) { }(), }, want: want{ - err: errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), context.DeadlineExceeded.Error()), + wantR: nil, + err: errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), context.DeadlineExceeded.Error()), }, } }(), @@ -462,10 +577,11 @@ func Test_redisClient_ping(t *testing.T) { client: test.fields.client, } - err := rc.ping(test.args.ctx) - if err := test.checkFunc(test.want, err); err != nil { + gotR, err := rc.ping(test.args.ctx) + if err := test.checkFunc(test.want, gotR, err); err != nil { tt.Errorf("error = %v", err) } + }) } } From 1249f508c1c158b2b9a234c31df70adceae95fc6 Mon Sep 17 00:00:00 2001 From: hlts2 Date: Thu, 30 Jul 2020 15:18:20 +0900 Subject: [PATCH 5/6] fix: apply suggestion Signed-off-by: hlts2 --- internal/db/kvs/redis/redis.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/db/kvs/redis/redis.go b/internal/db/kvs/redis/redis.go index 3828b92455..74ff306eef 100644 --- a/internal/db/kvs/redis/redis.go +++ b/internal/db/kvs/redis/redis.go @@ -169,13 +169,15 @@ func (rc *redisClient) ping(ctx context.Context) (r Redis, err error) { for { select { case <-pctx.Done(): - return nil, errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), pctx.Err().Error()) + err = errors.Wrap(errors.Wrap(err, errors.ErrRedisConnectionPingFailed.Error()), pctx.Err().Error()) + log.Error(err) + return nil, err case <-tick.C: err = rc.client.Ping().Err() if err == nil { return rc.client, nil } - log.Error(err) + log.Warn(err) } } } From 11417fcf73a971dbf3da153a612d7f6d022384ef Mon Sep 17 00:00:00 2001 From: vdaas-ci Date: Thu, 30 Jul 2020 06:39:47 +0000 Subject: [PATCH 6/6] :robot: Update license headers / Format go codes and yaml files Signed-off-by: vdaas-ci --- internal/db/kvs/redis/redis_mock_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/internal/db/kvs/redis/redis_mock_test.go b/internal/db/kvs/redis/redis_mock_test.go index e22cd9547c..47704bad70 100644 --- a/internal/db/kvs/redis/redis_mock_test.go +++ b/internal/db/kvs/redis/redis_mock_test.go @@ -1,3 +1,18 @@ +// +// Copyright (C) 2019-2020 Vdaas.org Vald team ( kpango, rinx, kmrmt ) +// +// 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 +// +// https://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 redis import redis "github.com/go-redis/redis/v7"