From f27f280c3fbb508293d6b0c5e72a1d91bd80f671 Mon Sep 17 00:00:00 2001 From: kevindiu Date: Mon, 13 Jul 2020 11:06:05 +0900 Subject: [PATCH 1/3] add cache test --- internal/cache/cache.go | 2 ++ internal/cache/cache_test.go | 57 ++++++++++++++++++++---------------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 49ed3ae461..92231dae93 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -26,6 +26,7 @@ import ( "github.com/vdaas/vald/internal/errors" ) +// Cache represent the cache interface to store cache type Cache interface { Start(context.Context) Get(string) (interface{}, bool) @@ -41,6 +42,7 @@ type cache struct { expiredHook func(context.Context, string) } +// New returns the Cache instance or error func New(opts ...Option) (cc Cache, err error) { c := new(cache) for _, opt := range append(defaultOpts, opts...) { diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 1e37531b4d..8adff64905 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -26,6 +26,13 @@ import ( "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"), + } +) + func TestNew(t *testing.T) { type args struct { opts []Option @@ -52,36 +59,36 @@ func TestNew(t *testing.T) { return nil } tests := []test{ - // TODO test cases - /* - { - name: "test_case_1", - args: args { - opts: nil, - }, - want: want{}, - checkFunc: defaultCheckFunc, - }, - */ + { + name: "return gache cacher", + args: args{ + opts: []Option{WithType("gache")}, + }, + checkFunc: func(w want, got Cache, err error) error { + if err != nil { + return err + } + if got == nil { + return errors.New("got cache is nil") + } - // TODO test cases - /* - func() test { - return test { - name: "test_case_2", - args: args { - opts: nil, - }, - want: want{}, - checkFunc: defaultCheckFunc, - } - }(), - */ + return nil + }, + }, + { + name: "return unknown error", + args: args{ + opts: []Option{WithType("unknown")}, + }, + want: want{ + err: errors.ErrInvalidCacherType, + }, + }, } for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(t) + defer goleak.VerifyNone(t, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } From abd7ba1532f86c1cfc564f7ca3afda3b52d12bb9 Mon Sep 17 00:00:00 2001 From: Kevin Diu Date: Mon, 13 Jul 2020 17:15:47 +0900 Subject: [PATCH 2/3] Update internal/cache/cache_test.go Co-authored-by: Kiichiro YUKAWA --- internal/cache/cache_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 8adff64905..6a056a2371 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -88,7 +88,7 @@ func TestNew(t *testing.T) { for _, test := range tests { t.Run(test.name, func(tt *testing.T) { - defer goleak.VerifyNone(t, goleakIgnoreOptions...) + defer goleak.VerifyNone(tt, goleakIgnoreOptions...) if test.beforeFunc != nil { test.beforeFunc(test.args) } From c991beaab4f2189adad737d4c291a06fd5ff793c Mon Sep 17 00:00:00 2001 From: kevindiu Date: Tue, 14 Jul 2020 12:03:56 +0900 Subject: [PATCH 3/3] fix --- internal/cache/cache.go | 5 +---- internal/cache/cache_test.go | 27 ++++++++++++++++++++++++++- internal/cache/option.go | 32 ++++++++++++-------------------- 3 files changed, 39 insertions(+), 25 deletions(-) diff --git a/internal/cache/cache.go b/internal/cache/cache.go index 92231dae93..41c9e4a345 100644 --- a/internal/cache/cache.go +++ b/internal/cache/cache.go @@ -46,10 +46,7 @@ type cache struct { func New(opts ...Option) (cc Cache, err error) { c := new(cache) for _, opt := range append(defaultOpts, opts...) { - err = opt(c) - if err != nil { - return nil, err - } + opt(c) } switch c.cacher { case cacher.GACHE: diff --git a/internal/cache/cache_test.go b/internal/cache/cache_test.go index 6a056a2371..daf7e431cf 100644 --- a/internal/cache/cache_test.go +++ b/internal/cache/cache_test.go @@ -76,7 +76,7 @@ func TestNew(t *testing.T) { }, }, { - name: "return unknown error", + name: "return unknown error when type is unknown", args: args{ opts: []Option{WithType("unknown")}, }, @@ -84,6 +84,31 @@ func TestNew(t *testing.T) { err: errors.ErrInvalidCacherType, }, }, + { + name: "return cache when type is empty", + args: args{ + opts: []Option{WithType("")}, + }, + checkFunc: func(w want, got Cache, err error) error { + if err != nil { + return err + } + if got == nil { + return errors.New("got cache is nil") + } + + return nil + }, + }, + { + name: "return unknown error when type is dummy string", + args: args{ + opts: []Option{WithType("dummy")}, + }, + want: want{ + err: errors.ErrInvalidCacherType, + }, + }, } for _, test := range tests { diff --git a/internal/cache/option.go b/internal/cache/option.go index b33490e8f1..18136535f7 100644 --- a/internal/cache/option.go +++ b/internal/cache/option.go @@ -21,11 +21,10 @@ import ( "context" "github.com/vdaas/vald/internal/cache/cacher" - "github.com/vdaas/vald/internal/errors" "github.com/vdaas/vald/internal/timeutil" ) -type Option func(*cache) error +type Option func(*cache) var ( defaultOpts = []Option{ @@ -36,52 +35,45 @@ var ( ) func WithExpiredHook(f func(context.Context, string)) Option { - return func(c *cache) error { + return func(c *cache) { if f != nil { c.expiredHook = f } - return nil } } func WithType(mo string) Option { - return func(c *cache) error { + return func(c *cache) { if len(mo) == 0 { - return nil + return } - m := cacher.ToType(mo) - if m == cacher.Unknown { - return errors.ErrInvalidCacherType - } - c.cacher = m - return nil + + c.cacher = cacher.ToType(mo) } } func WithExpireDuration(dur string) Option { - return func(c *cache) error { + return func(c *cache) { if len(dur) == 0 { - return nil + return } d, err := timeutil.Parse(dur) if err != nil { - return nil + return } c.expireDur = d - return nil } } func WithExpireCheckDuration(dur string) Option { - return func(c *cache) error { + return func(c *cache) { if len(dur) == 0 { - return nil + return } d, err := timeutil.Parse(dur) if err != nil { - return nil + return } c.expireCheckDur = d - return nil } }