Skip to content
15 changes: 6 additions & 9 deletions redis/cache/redispipebp/init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ import (
"os"
"testing"

"github.com/alicebob/miniredis/v2"
"github.com/joomcode/redispipe/redis"
"github.com/joomcode/redispipe/redisconn"

"github.com/reddit/baseplate.go/mqsend"
"github.com/reddit/baseplate.go/tracing"

"github.com/reddit/baseplate.go/redis/cache/redisx"
"github.com/reddit/baseplate.go/redis/cache/redisx/redisxtest"
)

var (
Expand All @@ -38,21 +38,18 @@ func TestMain(m *testing.M) {
panic(err)
}

s, err := miniredis.Run()
redisCluster, err := redisxtest.NewMockRedisCluster()
if err != nil {
panic(err)
}
defer s.Close()
defer redisCluster.Close()

sender, err := redisconn.Connect(context.TODO(), s.Addr(), redisconn.Opts{})
var clientTeardown func()
client, clientTeardown, err = redisxtest.NewMockRedisClient(context.TODO(), redisCluster.Addr(), redisconn.Opts{})
if err != nil {
panic(err)
}
defer sender.Close()

client = redisx.BaseSync{
SyncCtx: redis.SyncCtx{S: sender},
}
defer clientTeardown()

flushRedis()
os.Exit(m.Run())
Expand Down
4 changes: 4 additions & 0 deletions redis/cache/redisx/redisxtest/doc.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
// Package redisxtest provides utilities for baseplate.go/redids/cache/redisx testing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to spell out the full baseplate.go/redids/cache/redisx, just say "... for redisx testing" is enough.

//
// This includes functions for mocking redis instances.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment here regarding "mocking". I think with the current state of this change the first paragraph is enough for the package doc and we can just remove this paragraph.

package redisxtest
64 changes: 64 additions & 0 deletions redis/cache/redisx/redisxtest/redisxtest.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package redisxtest

import (
"context"

"github.com/alicebob/miniredis/v2"
"github.com/joomcode/redispipe/redis"
"github.com/joomcode/redispipe/redisconn"

"github.com/reddit/baseplate.go/redis/cache/redisx"
)

// MockRedisCluster wraps a local version of redis
type MockRedisCluster struct {
redisCluster *miniredis.Miniredis
}

func NewMockRedisCluster() (mockRedisCluster MockRedisCluster, err error) {
redisCluster, err := miniredis.Run()
if err != nil {
return MockRedisCluster{}, err
}

return MockRedisCluster{
redisCluster: redisCluster,
}, nil
}

// Addr returns address of mock redis cluster e.g. '127.0.0.1:12345'.
func (mrc *MockRedisCluster) Addr() string {
return mrc.redisCluster.Addr()
}

// Close shuts down the MockRedisCluster
func (mrc *MockRedisCluster) Close() error {
mrc.redisCluster.Close()
return nil
}

// NewMockRedisClient sets up a client and sender to a mock redis cluster
func NewMockRedisClient(
Comment on lines +13 to +14

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now there's nothing really "mocking" here (the mocking part is done via miniredis, which is no longer done here).

I would rename this function to NewTestRedisClient, and document that:

  1. the address arg should be coming from miniredis (we should also provide an example on how to it, see https://pkg.go.dev/testing#hdr-Examples, we also have prior arts in baseplate.go, for example https://github.com/reddit/baseplate.go/blob/master/errorsbp/batch_size_example_test.go)
  2. we should also document that the returned client will be auto released at the end of test and there's no need to do that explicitly (because the caller actually still have access to that via client.SyncCtx.S.Close()

ctx context.Context,
address string,
opts redisconn.Opts,
) (client redisx.BaseSync, teardown func(), err error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still recommend change this function signature into:

func NewMockRedisClient(ctx context.Context, tb testing.TB, address string, opts redisconn.Opts) redisx.BaseSync

it's OK for the mocked cluster to be a global singleton that's initialized in TestMain and shared between tests, because a mocked cluster is probably using much more resource and stand it up and tear it down in every test case is probably too slow/wasteful, but for clients, the pattern we should promote should be that every unit test create it and clean it up, instead of doing it in TestMain.


// Create connection
conn, err := redisconn.Connect(ctx, address, opts)
if err != nil {
return redisx.BaseSync{}, nil, err
}
Comment on lines +24 to +26

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh and this function no longer need to return an error. this can be just:

Suggested change
if err != nil {
return nil, err
}
if err != nil {
tb.Fatalf("Failed to create redis client: %v", err)
}


// Create client
client = redisx.BaseSync{
SyncCtx: redis.SyncCtx{S: conn},
}

// Teardown closure
teardown = func() {
conn.Close()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check the error and log it if it fails?

}

return client, teardown, nil
}