Close client connections after each redis test #654
Conversation
JoelSpeed
force-pushed
the
redis-test-client-close
branch
from
68faafb
to
dfa5bc9
Compare
pkg/sessions/redis/client.go
Outdated
|
|
||
| // close is only used in tests to ensure that client connections are terminated | ||
| // after each test is finished | ||
| close() error |
There was a problem hiding this comment.
Any advantage to just doing these in the tests themselves directly rather than adding to the interface and implementation just for tests sake?
redisStore.Client.(*client).Close()
And for cluster:
redisStore.Client.(*clusterClient).Close()
There was a problem hiding this comment.
I'm happy either way to be honest, it's not uncommon for things like this to be implemented for the tests sake within Go I don't think, but I could just do as you've suggested.
I deliberately set the method to private because it's only for testing so it's not on the public interface
There was a problem hiding this comment.
I just had another look and decided I will change it, added a small closer interface that can be used in the test, WDYT?
There was a problem hiding this comment.
Nice -- this looks super clean. I like it.
JoelSpeed
force-pushed
the
redis-test-client-close
branch
2 times, most recently
from
32e99d1
to
3d80d5b
Compare
| // Release any connections immediately after the test ends | ||
| if redisStore, ok := ss.(*SessionStore); ok { | ||
| if redisStore.Client != nil { | ||
| redisStore.Client.(closer).Close() |
There was a problem hiding this comment.
Since Close() can error - I think we should assert that it didn't error.
Otherwise someday some developer will run into the same unclear socket timeout/file descriptor errors that are less clear when we could expose more clearly that the error was here.
There was a problem hiding this comment.
Good catch, gimme a second
JoelSpeed
force-pushed
the
redis-test-client-close
branch
from
3d80d5b
to
5c8a66b
Compare
Description
Make sure we close connections in the redis client after each test
Motivation and Context
Fixes #653
How Has This Been Tested?
docker run --rm -it -v $PWD:/go/src/github.com/oauth2-proxy/oauth2-proxy --workdir /go/src/github.com/oauth2-proxy/oauth2-proxy/pkg/sessions/redis golang:1.14 bash -c "ulimit -n 256 && ulimit -a && go test -v ./..."Checklist: