Add redisxtest #484

zack-keim · 2022-02-28T23:22:46Z

Summary

Adds redisxtest implementation which provides setup and teardown functions for setting up a mock redis cluster and client for testing purposes. One of the challenges of using this package is that testing clients requires writing setup and teardown functions for the mock redis instance. This leads to a lot of copy and paste across baseplate.go services, since the pattern is nearly identical. This change makes it possible to set up a mock redis cluster and client of that cluster with two simple functions.

What to look out for

Current implementation uses miniredis to mock the redis instance. miniredis is the currently the predominately used library for standing up a local in-memory redis instance for testing purposes in go. However, there may be other options out there.

konradreiche

Could we apply this to existing test code in this repository while we are at it? I noticed that we use miniredis here too:

fishy · 2022-03-01T22:05:48Z

redis/cache/redisx/redisxtest/redisxtest.go

+func NewMockRedisClient(ctx context.Context, timeout time.Duration) (client redisx.Syncx, teardown func(), err error) {
+	redisCluster, err := miniredis.Run()
+	if err != nil {
+		return redisx.Syncx{}, func() {}, err


nit: in an error return, we would want to return nil for non-error returns whenever possible. you cannot return nil for the client, but you certainly can return nil for the teardown (we don't want people using it to blindly defer teardown() without checking for errors first).

fishy · 2022-03-01T22:07:21Z

redis/cache/redisx/redisxtest/redisxtest.go

+
+// NewMockRedisClient sets up a mock redis cluster, client and sender
+// This should be called from TestMain since the miniredis instance created is locked
+func NewMockRedisClient(ctx context.Context, timeout time.Duration) (client redisx.Syncx, teardown func(), err error) {


if this is only intended to be used by unit tests, we do not really need to return teardown and err. just pass intb testing.TB as an arg (so it can be used by both tests and benchmarks), then when an error happens we can just tb.Fatalf("Failed to ...: %v", err), and at the end, before returning, we can just call tb.Cleanup(teardown).

Looks like the main use case is to set this up in testing.M which sadly does not have those those methods available.

@konradreiche that is correct. We have been following the pattern of setup in TestMain since places where the mock redis cluster is used tend to be places where it made sense to init there. This approach also gives the flexibility to use this func in both tests directly but also in TestMain. However, if we feel strongly that supporting TestMain doesn't have much value, then I am happy to change this.

fishy · 2022-03-01T22:08:14Z

redis/cache/redisx/redisxtest/redisxtest.go

+	return client, teardown, nil
+}
+
+func FlushRedis(ctx context.Context, client redisx.Syncx) error {


I don't see this used by NewMockRedisClient, and it's not explained by the PR description either.

Removing. This was a func we were using locally that got dropped in here. Agree it is probably not necessary.

fishy · 2022-03-02T22:26:54Z

redis/cache/redispipebp/init_test.go

@@ -53,7 +60,9 @@ func TestMain(m *testing.M) {
 	client = redisx.BaseSync{


this value of client is no longer used by anything?

fishy · 2022-03-02T22:31:59Z

redis/cache/redisx/redisxtest/redisxtest.go

+// NewMockRedisClient sets up a client and sender to a mock redis cluster
+func NewMockRedisClient(
+	ctx context.Context,
+	redisCluster MockRedisCluster,


this feels like an over-design to me.

if in most tests we only need a single, global, mocked cluster and client, then we don't really need separated NewMockRedisCluster and NewMockRedisClient, a single NewMockRedisClient with its returned teardown also tears down the cluster should be sufficient (the cluster does not need to be exposed to the caller at all).

but if that's not the case, then I think only the cluster itself should be global (that's handled inside TestMain), the clients should be created per test, so my previous comment regarding passing in testing.TB and no need to return error nor teardown applies.

and looking at this function again I don't see it doing anything special. all it does are pretty standard stuff about create a redis connection (you actually only need the address passed in, not the whole cluster object) and create a redis client out of this connection. the teardown is also standard stuff about closing the client, so I don't really see much value of providing (and maintaining) this as part of the public API.

If we think a func NewClient(ctx, addr, opts) (redisx.BaseSync, error) API is useful, then that should be part of redisx, not redisxtest.

Reason for the change to have separated NewMockRedisCluster and NewMockRedisClient was to support the cases like that in TestMain in init_test, where it looks like we need a global cluster. Perhaps an alternative solution to this would be to have the following API for redisxtest:

func NewMockRedisClient(tb, ctx, connOpts) (client, error) func NewMockRedisClientWithCluster(tb, ctx, clusterAddr, connOpts) (client, error)

These functions would handle cleanup and client creation via tb as you have suggested.

I am really not opposed at all to going down the path updating the redisx API to have a func NewClient(). This is actually our preferred route. We saw adding redisxtest as being minimally invasive, which was the reason for going this path. If you are ok with us doing that, happy to open another PR for that change and close potentially close this one if we see no value in what I have proposed.

fishy · 2022-03-02T22:32:40Z

redis/cache/redisx/redisxtest/redisxtest.go

+	// Create connection
+	conn, err := redisconn.Connect(ctx, redisCluster.Addr(), opts)
+	if err != nil {
+		redisCluster.Close()


this is a very unexpected side effect. if a client cannot connect to the server, it will also kill the server, causing other, clients to stop working.

fishy · 2022-03-02T22:50:02Z

redis/cache/redisx/redisxtest/redisxtest.go

+}
+
+// Close shuts down the MockRedisCluster
+func (mrc *MockRedisCluster) Close() {


nit: make this function to return an error, so it implements io.Closer interface

since miniredis.Miniredis.Close does not return an error, you can just always return nil inside this function.

fishy · 2022-03-02T22:51:36Z

redis/cache/redisx/redisxtest/redisxtest.go

+	teardown = func() {
+		mockRedisCluster.Close()
+	}


this is a very standard closing pattern in go (especially when you make MockRedisCluster to implement io.Closer), so I would say returning a teardown is unnecessary, caller could just do:

cluster, err := NewMockRedisCluster() if err != nil { ... } defer cluster.Close()

fishy · 2022-03-02T22:57:48Z

redis/cache/redisx/redisxtest/redisxtest.go

+// NewMockRedisClient sets up a client and sender to a mock redis cluster
+func NewMockRedisClient(
+	ctx context.Context,
+	redisCluster MockRedisCluster,


and looking at this function again I don't see it doing anything special. all it does are pretty standard stuff about create a redis connection (you actually only need the address passed in, not the whole cluster object) and create a redis client out of this connection. the teardown is also standard stuff about closing the client, so I don't really see much value of providing (and maintaining) this as part of the public API.

If we think a func NewClient(ctx, addr, opts) (redisx.BaseSync, error) API is useful, then that should be part of redisx, not redisxtest.

fishy · 2022-03-08T18:29:44Z

redis/cache/redisx/redisxtest/redisxtest.go

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


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.

fishy

we would still want to change the existing tests to use this, that includes:

instead of store the client globally (

baseplate.go/redis/cache/redispipebp/init_test.go

Line 19 in a6f0c8a

client redisx.Sync

), we store the miniredis address globally
in tests currently using the global client, use this function to get the client instead
change flushRedis to take tb and client as args.

fishy · 2022-03-09T21:15:18Z

redis/cache/redisx/redisxtest/redisxtest.go

+	// Cleanup
+	tb.Cleanup(func() {
+		conn.Close()
+	})


nit: I would move this up to right after the err check (to line 27).

fishy · 2022-03-09T21:20:46Z

redis/cache/redisx/redisxtest/redisxtest.go

+// NewMockRedisClient sets up a redis client for use in testing
+func NewMockRedisClient(


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:

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)

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()

fishy · 2022-03-09T21:21:14Z

redis/cache/redisx/redisxtest/doc.go

@@ -0,0 +1,4 @@
+// Package redisxtest provides utilities for baseplate.go/redids/cache/redisx testing.


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

fishy · 2022-03-09T21:21:43Z

redis/cache/redisx/redisxtest/doc.go

@@ -0,0 +1,4 @@
+// Package redisxtest provides utilities for baseplate.go/redids/cache/redisx testing.
+//
+// This includes functions for mocking redis instances.


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.

fishy · 2022-03-09T21:25:17Z

redis/cache/redisx/redisxtest/redisxtest.go

+	tb testing.TB,
+	address string,
+	opts redisconn.Opts,
+) (*redisx.BaseSync, error) {


looking at how existing tests work, we do need the returned client to be of type redisx.BaseSync instead of the pointer, for example

baseplate.go/redis/cache/redispipebp/errors_test.go

Line 57 in a6f0c8a

wClient := redispipebp.WrapErrorsSync{Sync: client}

fishy · 2022-03-09T21:28:13Z

redis/cache/redisx/redisxtest/redisxtest.go

+	if err != nil {
+		return nil, err
+	}


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)

}

kylelemons · 2022-03-15T23:06:09Z

redis/cache/redisx/redisxtest/redisxtest.go

+	// Create connection to redis
+	conn, err := redisconn.Connect(ctx, address, opts)
+	if err != nil {
+		return nil, err


If this is a testing helper, in my opinion it's better to call t.Fatalf and document that it must be called directly in a test / subtest, rather than do what is inevitable: have every user of this make their own local helper that wraps this helper that does the transition. I prefix my t.Fatalf errors with "SETUP:" to make it clear that it's not a failure of the code under test, and I would name the function "Setup" not "New" to encourage that interpretation as well.

kylelemons · 2022-03-15T23:06:21Z

redis/cache/redisx/redisxtest/redisxtest.go

+
+	// Cleanup
+	tb.Cleanup(func() {
+		conn.Close()


check the error and log it if it fails?

zack-keim added 5 commits February 28, 2022 14:37

Add redistest

0bcd4fb

Add redistest documentation

1046c61

Move redistest to redisx

109ab82

Rename package to redisxtest

6149f7f

Revise redisxtest document

94e25e8

zack-keim changed the title ~~Add Redistest~~ Add redisxtest Feb 28, 2022

zack-keim marked this pull request as ready for review March 1, 2022 00:50

zack-keim requested a review from a team as a code owner March 1, 2022 00:50

zack-keim requested review from fishy, kylelemons and konradreiche and removed request for a team March 1, 2022 00:50

konradreiche requested changes Mar 1, 2022

View reviewed changes

fishy requested changes Mar 1, 2022

View reviewed changes

zack-keim added 5 commits March 2, 2022 13:40

Return nil teardown in error case

6443170

Remove FlushRedis

b9a7d47

Add MockRedisCluster wrapper

a07bb69

Fix up comments

a3a0caa

Refactor init_test to use redisxtest

d66f70d

zack-keim requested review from konradreiche and fishy March 2, 2022 22:21

fishy requested changes Mar 2, 2022

View reviewed changes

zack-keim added 2 commits March 2, 2022 14:33

Remove old client init

f638087

Remove cluster close

124335f

fishy requested changes Mar 2, 2022

View reviewed changes

Make MockRedisCluster satisfy closer interface

acd77b3

zack-keim requested a review from fishy March 3, 2022 01:06

konradreiche approved these changes Mar 8, 2022

View reviewed changes

fishy requested changes Mar 8, 2022

View reviewed changes

Revert changes to init_test.go

481b431

zack-keim requested a review from fishy March 9, 2022 20:18

fishy requested changes Mar 9, 2022

View reviewed changes

fishy reviewed Mar 9, 2022

View reviewed changes

kylelemons requested changes Mar 15, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add redisxtest #484

Add redisxtest #484

zack-keim commented Feb 28, 2022 •

edited

Loading

konradreiche left a comment •

edited

Loading

fishy Mar 1, 2022

fishy Mar 1, 2022

konradreiche Mar 2, 2022

zack-keim Mar 2, 2022

fishy Mar 1, 2022

zack-keim Mar 2, 2022

fishy Mar 2, 2022

zack-keim Mar 2, 2022

fishy Mar 2, 2022

fishy Mar 2, 2022

zack-keim Mar 3, 2022 •

edited

Loading

fishy Mar 2, 2022

zack-keim Mar 2, 2022

zack-keim Mar 3, 2022

fishy Mar 2, 2022

fishy Mar 2, 2022

fishy Mar 2, 2022

fishy Mar 8, 2022

fishy left a comment

fishy Mar 9, 2022

fishy Mar 9, 2022

fishy Mar 9, 2022

fishy Mar 9, 2022

fishy Mar 9, 2022

fishy Mar 9, 2022

kylelemons Mar 15, 2022

kylelemons Mar 15, 2022

		@@ -53,7 +60,9 @@ func TestMain(m *testing.M) {
		client = redisx.BaseSync{

		// NewMockRedisClient sets up a redis client for use in testing
		func NewMockRedisClient(

		@@ -0,0 +1,4 @@
		// Package redisxtest provides utilities for baseplate.go/redids/cache/redisx testing.

Add redisxtest #484

Are you sure you want to change the base?

Add redisxtest #484

Conversation

zack-keim commented Feb 28, 2022 • edited Loading

Summary

What to look out for

konradreiche left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zack-keim Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fishy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zack-keim commented Feb 28, 2022 •

edited

Loading

konradreiche left a comment •

edited

Loading

zack-keim Mar 3, 2022 •

edited

Loading