Skip to content

Add redisxtest #484

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

Open
zack-keim 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 5 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
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.

@fishy fishy Mar 9, 2022

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.

@fishy fishy Mar 9, 2022

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
43 changes: 43 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,43 @@
package redisxtest

import (
"context"
"time"

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

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

// 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) {

@fishy fishy Mar 1, 2022

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.

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

@konradreiche konradreiche Mar 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@zack-keim zack-keim Mar 2, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@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.

redisCluster, err := miniredis.Run()
if err != nil {
return redisx.Syncx{}, func() {}, err

@fishy fishy Mar 1, 2022

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.

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

}
Comment on lines +24 to +26

@fishy fishy Mar 9, 2022

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


conn, err := redisconn.Connect(ctx, redisCluster.Addr(), redisconn.Opts{IOTimeout: timeout})
if err != nil {
redisCluster.Close()

@fishy fishy Mar 2, 2022

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.

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.

@zack-keim zack-keim Mar 2, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed.

@zack-keim zack-keim Mar 3, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed

return redisx.Syncx{}, func() {}, err
}

client = redisx.Syncx{
Sync: redisx.BaseSync{SyncCtx: redis.SyncCtx{S: conn}},
}

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

@kylelemons kylelemons Mar 15, 2022

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
}

func FlushRedis(ctx context.Context, client redisx.Syncx) error {

@fishy fishy Mar 1, 2022

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 don't see this used by NewMockRedisClient, and it's not explained by the PR description either.

@zack-keim zack-keim Mar 2, 2022

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

return client.Send(ctx, redisx.Req(nil, "FLUSHALL"))
}