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 all 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.
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
39 changes: 39 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,39 @@
package redisxtest

import (
"context"
"testing"

"github.com/joomcode/redispipe/redis"
"github.com/joomcode/redispipe/redisconn"

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

// NewMockRedisClient sets up a redis client for use in testing
func NewMockRedisClient(
Comment on lines +13 to +14
Copy link
Member

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,
tb testing.TB,
address string,
opts redisconn.Opts,
) (*redisx.BaseSync, error) {
Copy link
Member

Choose a reason for hiding this comment

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

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}

tb.Helper()

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

Choose a reason for hiding this comment

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

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.

}
Comment on lines +24 to +26
Copy link
Member

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

// Cleanup
tb.Cleanup(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?

})
Comment on lines +33 to +36
Copy link
Member

Choose a reason for hiding this comment

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

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


return &client, nil
}