Repository: zeromicro/go-zero
Pull request #3338 (2023-06-11 14:00:10)

chore: add more tests

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/zeromicro/go-zero/pull/3338?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#3338](https://app.codecov.io/gh/zeromicro/go-zero/pull/3338?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (44426e5) into [master](https://app.codecov.io/gh/zeromicro/go-zero/commit/efa6940001c8da02a58552bd55ac84cbafbbe6f3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (efa6940) will **increase** coverage by `0.07%`.
> The diff coverage is `87.50%`.


| [Impacted Files](https://app.codecov.io/gh/zeromicro/go-zero/pull/3338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [core/proc/signals.go](https://app.codecov.io/gh/zeromicro/go-zero/pull/3338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y29yZS9wcm9jL3NpZ25hbHMuZ28=) | `43.33% <0.00%> (ø)` | |
| [core/proc/goroutines.go](https://app.codecov.io/gh/zeromicro/go-zero/pull/3338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y29yZS9wcm9jL2dvcm91dGluZXMuZ28=) | `100.00% <100.00%> (+21.42%)` | :arrow_up: |
| [core/search/tree.go](https://app.codecov.io/gh/zeromicro/go-zero/pull/3338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-Y29yZS9zZWFyY2gvdHJlZS5nbw==) | `100.00% <100.00%> (+0.79%)` | :arrow_up: |

... and [2 files with indirect coverage changes](https://app.codecov.io/gh/zeromicro/go-zero/pull/3338/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)


Patch:
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/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/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/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/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]


Test patch:
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_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_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_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/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()