[Cache Proxy] add an integration test #7966
Conversation
| CacheEncryptionEnabled bool | ||
| } | ||
|
|
||
| func WithTestRedis(t *testing.T) *Options { |
There was a problem hiding this comment.
nit: it'll be awkward if we want to add more functions like this later:
redisOpts := enterprise_testenv.WithTestRedis(t)
fooOpts := enterprise_testenv.WithFoo(t)
opts := &enterprise_testenv.Options{
RedisTarget: redisOpts.RedisTarget,
Foo: fooOpts.Foo,
}
so, I think we should avoid this pattern and let the caller manage the Options struct:
opts := &Options{
RedisTarget: testredis.Start(t).Target,
Foo: testfoo.New(t),
}
| auth := enterprise_testauth.Configure(t, testEnv) | ||
| flags.Set(t, "auth.jwt_key", auth.GetJwtKey()) |
There was a problem hiding this comment.
would it make sense to do this in enterprise_testauth.Configure() so that we don't have to introduce GetJwtKey() on TestAuthenticator?
| @@ -18,7 +19,13 @@ import ( | |||
| ) | |||
|
|
|||
| type Options struct { | |||
| RedisTarget string | |||
| RedisTarget string | |||
| CacheEncryptionEnabled bool | |||
There was a problem hiding this comment.
this CacheEncryptionEnabled field is only used in rbetest, and is unused in GetCustomTestEnv, so I think it's a bit confusing to add it here. Instead of updating this opts struct, maybe add a separate options struct to rbetest?
| @@ -57,7 +57,7 @@ func Configure(t *testing.T, env *real_environment.RealEnv) *testauth.TestAuthen | |||
|
|
|||
| // CreateRandomGroups creates several randomly generated orgs with several | |||
| // randomly generated users under each. | |||
| func CreateRandomGroups(t *testing.T, env environment.Env) []*tables.User { | |||
| func CreateRandomGroups(t *testing.T, env environment.Env, encryptionEnabled bool) []*tables.User { | |||
There was a problem hiding this comment.
The newly added test only seems to be testing GR1 - could encryption be left disabled for the random groups? The test could do something like env.GetUserDB().UpdateGroup(...) to enable encryption just for GR1.
Asking because it feels a little inconvenient for this to be a mandatory parameter, since in reality encryption is somewhat of a special case.
Only one test case for now that tests the bug fixed in this PR: #7852