From f6bdb6e1de8c855fb097a75f4ac4413e736c337a Mon Sep 17 00:00:00 2001 From: Kevin Wan Date: Mon, 12 Jun 2023 01:22:20 +0800 Subject: [PATCH] chore: add more tests (#3338) --- core/proc/goroutines+polyfill.go | 6 ---- core/proc/goroutines.go | 14 ++++++-- core/proc/goroutines_test.go | 32 ++++++++++++++++-- core/proc/signals.go | 2 +- core/proc/signals_test.go | 2 ++ core/search/tree.go | 6 ++-- core/search/tree_test.go | 8 ++++- core/stores/sqlc/cachedsql.go | 9 +++--- core/stores/sqlc/cachedsql_test.go | 52 ++++++++++++++++-------------- core/syncx/resourcemanager.go | 3 +- internal/health/health_test.go | 13 ++++++++ 11 files changed, 100 insertions(+), 47 deletions(-) delete mode 100644 core/proc/goroutines+polyfill.go diff --git a/core/proc/goroutines+polyfill.go b/core/proc/goroutines+polyfill.go deleted file mode 100644 index cf4c90cf..00000000 --- a/core/proc/goroutines+polyfill.go +++ /dev/null @@ -1,6 +0,0 @@ -//go:build windows - -package proc - -func dumpGoroutines() { -} diff --git a/core/proc/goroutines.go b/core/proc/goroutines.go index 22799374..891150de 100644 --- a/core/proc/goroutines.go +++ b/core/proc/goroutines.go @@ -18,7 +18,11 @@ const ( debugLevel = 2 ) -func dumpGoroutines() { +type creator interface { + Create(name string) (file *os.File, err error) +} + +func dumpGoroutines(ctor creator) { command := path.Base(os.Args[0]) pid := syscall.Getpid() dumpFile := path.Join(os.TempDir(), fmt.Sprintf("%s-%d-goroutines-%s.dump", @@ -26,10 +30,16 @@ func dumpGoroutines() { logx.Infof("Got dump goroutine signal, printing goroutine profile to %s", dumpFile) - if f, err := os.Create(dumpFile); err != nil { + if f, err := ctor.Create(dumpFile); err != nil { logx.Errorf("Failed to dump goroutine profile, error: %v", err) } else { defer f.Close() pprof.Lookup(goroutineProfile).WriteTo(f, debugLevel) } } + +type fileCreator struct{} + +func (fc fileCreator) Create(name string) (file *os.File, err error) { + return os.Create(name) +} diff --git a/core/proc/goroutines_test.go b/core/proc/goroutines_test.go index 267264c3..827d0233 100644 --- a/core/proc/goroutines_test.go +++ b/core/proc/goroutines_test.go @@ -1,6 +1,10 @@ +//go:build linux || darwin + package proc import ( + "errors" + "os" "strings" "testing" @@ -9,7 +13,29 @@ import ( ) func TestDumpGoroutines(t *testing.T) { - buf := logtest.NewCollector(t) - dumpGoroutines() - assert.True(t, strings.Contains(buf.String(), ".dump")) + t.Run("real file", func(t *testing.T) { + buf := logtest.NewCollector(t) + dumpGoroutines(fileCreator{}) + assert.True(t, strings.Contains(buf.String(), ".dump")) + }) + + t.Run("fake file", func(t *testing.T) { + const msg = "any message" + buf := logtest.NewCollector(t) + err := errors.New(msg) + dumpGoroutines(fakeCreator{ + file: &os.File{}, + err: err, + }) + assert.True(t, strings.Contains(buf.String(), msg)) + }) +} + +type fakeCreator struct { + file *os.File + err error +} + +func (fc fakeCreator) Create(name string) (file *os.File, err error) { + return fc.file, fc.err } diff --git a/core/proc/signals.go b/core/proc/signals.go index 6851cff8..2f9af2d1 100644 --- a/core/proc/signals.go +++ b/core/proc/signals.go @@ -26,7 +26,7 @@ func init() { v := <-signals switch v { case syscall.SIGUSR1: - dumpGoroutines() + dumpGoroutines(fileCreator{}) case syscall.SIGUSR2: if profiler == nil { profiler = StartProfile() diff --git a/core/proc/signals_test.go b/core/proc/signals_test.go index e46cfdb2..c49d503b 100644 --- a/core/proc/signals_test.go +++ b/core/proc/signals_test.go @@ -1,3 +1,5 @@ +//go:build linux || darwin + package proc import ( diff --git a/core/search/tree.go b/core/search/tree.go index 34741077..960f3f3f 100644 --- a/core/search/tree.go +++ b/core/search/tree.go @@ -171,11 +171,11 @@ func add(nd *node, route string, item any) error { token := route[:i] children := nd.getChildren(token) if child, ok := children[token]; ok { - if child != nil { - return add(child, route[i+1:], item) + if child == nil { + return errInvalidState } - return errInvalidState + return add(child, route[i+1:], item) } child := newNode(nil) diff --git a/core/search/tree_test.go b/core/search/tree_test.go index 3f711e76..1bad563a 100644 --- a/core/search/tree_test.go +++ b/core/search/tree_test.go @@ -11,7 +11,7 @@ import ( type mockedRoute struct { route string - value int + value any } func TestSearch(t *testing.T) { @@ -187,6 +187,12 @@ func TestSearchInvalidItem(t *testing.T) { assert.Equal(t, errEmptyItem, err) } +func TestSearchInvalidState(t *testing.T) { + nd := newNode("0") + nd.children[0]["1"] = nil + assert.Error(t, add(nd, "1/2", "2")) +} + func BenchmarkSearchTree(b *testing.B) { const ( avgLen = 1000 diff --git a/core/stores/sqlc/cachedsql.go b/core/stores/sqlc/cachedsql.go index 31433070..8b81e925 100644 --- a/core/stores/sqlc/cachedsql.go +++ b/core/stores/sqlc/cachedsql.go @@ -97,6 +97,9 @@ func (cc CachedConn) Exec(exec ExecFn, keys ...string) (sql.Result, error) { } // ExecCtx runs given exec on given keys, and returns execution result. +// If DB operation succeeds, it will delete cache with given keys, +// if DB operation fails, it will return nil result and non-nil error, +// if DB operation succeeds but cache deletion fails, it will return result and non-nil error. func (cc CachedConn) ExecCtx(ctx context.Context, exec ExecCtxFn, keys ...string) ( sql.Result, error) { res, err := exec(ctx, cc.db) @@ -104,11 +107,7 @@ func (cc CachedConn) ExecCtx(ctx context.Context, exec ExecCtxFn, keys ...string return nil, err } - if err := cc.DelCacheCtx(ctx, keys...); err != nil { - return nil, err - } - - return res, nil + return res, cc.DelCacheCtx(ctx, keys...) } // ExecNoCache runs exec with given sql statement, without affecting cache. diff --git a/core/stores/sqlc/cachedsql_test.go b/core/stores/sqlc/cachedsql_test.go index 41bb17d6..c71ddcb5 100644 --- a/core/stores/sqlc/cachedsql_test.go +++ b/core/stores/sqlc/cachedsql_test.go @@ -471,31 +471,33 @@ func TestCachedConnExec(t *testing.T) { } func TestCachedConnExecDropCache(t *testing.T) { - r, err := miniredis.Run() - assert.Nil(t, err) - defer fx.DoWithTimeout(func() error { - r.Close() - return nil - }, time.Second) - - const ( - key = "user" - value = "any" - ) - var conn trackedConn - c := NewNodeConn(&conn, redis.New(r.Addr()), cache.WithExpiry(time.Second*30)) - assert.Nil(t, c.SetCache(key, value)) - _, err = c.Exec(func(conn sqlx.SqlConn) (result sql.Result, e error) { - return conn.Exec("delete from user_table where id='kevin'") - }, key) - assert.Nil(t, err) - assert.True(t, conn.execValue) - _, err = r.Get(key) - assert.Exactly(t, miniredis.ErrKeyNotFound, err) - _, err = c.Exec(func(conn sqlx.SqlConn) (result sql.Result, e error) { - return nil, errors.New("foo") - }, key) - assert.NotNil(t, err) + t.Run("drop cache", func(t *testing.T) { + r, err := miniredis.Run() + assert.Nil(t, err) + defer fx.DoWithTimeout(func() error { + r.Close() + return nil + }, time.Second) + + const ( + key = "user" + value = "any" + ) + var conn trackedConn + c := NewNodeConn(&conn, redis.New(r.Addr()), cache.WithExpiry(time.Second*30)) + assert.Nil(t, c.SetCache(key, value)) + _, err = c.Exec(func(conn sqlx.SqlConn) (result sql.Result, e error) { + return conn.Exec("delete from user_table where id='kevin'") + }, key) + assert.Nil(t, err) + assert.True(t, conn.execValue) + _, err = r.Get(key) + assert.Exactly(t, miniredis.ErrKeyNotFound, err) + _, err = c.Exec(func(conn sqlx.SqlConn) (result sql.Result, e error) { + return nil, errors.New("foo") + }, key) + assert.NotNil(t, err) + }) } func TestCachedConn_SetCacheWithExpire(t *testing.T) { diff --git a/core/syncx/resourcemanager.go b/core/syncx/resourcemanager.go index 66390c8a..b4a6a729 100644 --- a/core/syncx/resourcemanager.go +++ b/core/syncx/resourcemanager.go @@ -42,7 +42,8 @@ func (manager *ResourceManager) Close() error { } // GetResource returns the resource associated with given key. -func (manager *ResourceManager) GetResource(key string, create func() (io.Closer, error)) (io.Closer, error) { +func (manager *ResourceManager) GetResource(key string, create func() (io.Closer, error)) ( + io.Closer, error) { val, err := manager.singleFlight.Do(key, func() (any, error) { manager.lock.RLock() resource, ok := manager.resources[key] diff --git a/internal/health/health_test.go b/internal/health/health_test.go index 07e8779b..4bdf7e2c 100644 --- a/internal/health/health_test.go +++ b/internal/health/health_test.go @@ -53,6 +53,19 @@ func TestComboHealthManager(t *testing.T) { assert.True(t, chm.IsReady()) }) + t.Run("is ready verbose", func(t *testing.T) { + chm := newComboHealthManager() + hm := NewHealthManager(probeName) + + assert.True(t, chm.IsReady()) + chm.addProbe(hm) + assert.False(t, chm.IsReady()) + hm.MarkReady() + assert.True(t, chm.IsReady()) + assert.Contains(t, chm.verboseInfo(), probeName) + assert.Contains(t, chm.verboseInfo(), "is ready") + }) + t.Run("concurrent add probes", func(t *testing.T) { chm := newComboHealthManager()