Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add redisxtest #484

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add redisxtest #484

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
0bcd4fb
Add redistest
zack-keim Feb 28, 2022
1046c61
Add redistest documentation
zack-keim Feb 28, 2022
109ab82
Move redistest to redisx
zack-keim Feb 28, 2022
6149f7f
Rename package to redisxtest
zack-keim Feb 28, 2022
94e25e8
Revise redisxtest document
zack-keim Feb 28, 2022
6443170
Return nil teardown in error case
zack-keim Mar 2, 2022
b9a7d47
Remove FlushRedis
zack-keim Mar 2, 2022
a07bb69
Add MockRedisCluster wrapper
zack-keim Mar 2, 2022
a3a0caa
Fix up comments
zack-keim Mar 2, 2022
d66f70d
Refactor init_test to use redisxtest
zack-keim Mar 2, 2022
f638087
Remove old client init
zack-keim Mar 2, 2022
124335f
Remove cluster close
zack-keim Mar 2, 2022
acd77b3
Make MockRedisCluster satisfy closer interface
zack-keim Mar 3, 2022
481b431
Revert changes to init_test.go
zack-keim Mar 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion redis/cache/redispipebp/init_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"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 Down Expand Up @@ -44,6 +45,12 @@ func TestMain(m *testing.M) {
}
defer s.Close()

redisCluster, clusterTeardown, err := redisxtest.NewMockRedisCluster()
if err != nil {
panic(err)
}
defer clusterTeardown()

sender, err := redisconn.Connect(context.TODO(), s.Addr(), redisconn.Opts{})
if err != nil {
panic(err)
Expand All @@ -53,7 +60,9 @@ func TestMain(m *testing.M) {
client = redisx.BaseSync{
Copy link
Member

Choose a reason for hiding this comment

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

this value of client is no longer used by anything?

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

SyncCtx: redis.SyncCtx{S: sender},
}

var clientTeardown func()
client, clientTeardown, err = redisxtest.NewMockRedisClient(context.TODO(), redisCluster, redisconn.Opts{})
defer clientTeardown()
flushRedis()
os.Exit(m.Run())
}
Expand Down
4 changes: 4 additions & 0 deletions redis/cache/redisx/redisxtest/doc.go
View file
Open in desktop
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
Member

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
Member

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
70 changes: 70 additions & 0 deletions redis/cache/redisx/redisxtest/redisxtest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
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, teardown func(), err error) {
redisCluster, err := miniredis.Run()
if err != nil {
return MockRedisCluster{}, nil, err
}

mockRedisCluster = MockRedisCluster{
redisCluster: redisCluster,
}

teardown = func() {
mockRedisCluster.Close()
}
Copy link
Member

Choose a reason for hiding this comment

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

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


return mockRedisCluster, teardown, 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() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

mrc.redisCluster.Close()
}

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@zack-keim zack-keim Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

opts redisconn.Opts,
) (client redisx.BaseSync, teardown func(), err error) {

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

Copy link
Author

Choose a reason for hiding this comment

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

Removed

return redisx.BaseSync{}, nil, err
}

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

// Teardown closure
teardown = func() {
conn.Close()
Copy link
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
}