Record and rollback keys in a test

[sjurgemeyer]

This change intends to make it easier to keep some data populated in redis between
integration tests, while still avoiding test pollution.

I've added TestRedisService which is injected automatically instead of
RedisService during the test phase. TestRedisService is just a wrapper
around RedisService that allows you to turn on recording of inserted keys and
then call a method to delete them all when you're done.

[sjurgemeyer]

There was some mixed tabs and spaces in this file.

[sjurgemeyer]

RedisService was moved to src as it's now being wired manually. If it was left in services, grails would override the wiring done in the doWithSpring block for 'redisService'

[tednaleid]

Thanks for the pull request Shaun, it's an interesting idea. I'd like to understand the use-case a little more. How is the data that we don't want to wipe out getting into redis? Do you have some sort of bootstrap process or something that you're running ahead of time and leaving in place for all tests?

Overall, the code in the PR looks good, I'm just a little worried about the maintainability and places where there could have been changes that we can't easily monitor through this technique (new commands, or side effects from things like Lua scripts that get run).

Redis' interface has been pretty stable these days, so that might not be that big of a concern.

[sjurgemeyer]

In our case we have a lot of data that is bootstrapped into redis that we would rather not flush and repopulate in every test. So the idea is to bootstrap certain data and then let tests extend a common integration spec that runs this code to clean up after the fact. Currently we are forced to do that in individual tests which is very error prone.

I agree that there is a lot of room for other keys leaking in here. As I mention in the javadoc for the TestRedisService, I basically ignored any methods that weren't direct 'set' methods since depending on your use case, you may not want to clear the whole key. That makes this whole thing a little more ambiguous than I'd prefer, but I think overall it's still useful.

The good thing is that it won't break any existing tests since you have to explicitly call these methods for it to have any change to functionality. I think it's worthwhile, but I understand if you'd like to mull it over or get some other opinions on it.

Let me know if you have any thoughts on how to make this better.

[tednaleid]

As I understand the code currently, if you set/modify a key, it'll register that key for deletion, even if it had a (potentially different) value prior to the bootstrap.

As test order isn't guaranteed to be the same, how should that be dealt with in the tests? Are tests responsible for also putting the values they change that existed previously back to the values that existed prior to the test?

If this is an issue, one idea that comes to mind is that when a new value is about to get added to the recordedKeys set, that if it already exists in redis the value could be DUMPed out and then later RESTOREd.

Are you using either the withPipeline or withTransaction closure methods in your code? I think that because these don't do redis operations directly on the RedisService, but instead on Pipeline and Transaction objects that no keys would get recorded during those methods.

I don't have any great ideas that are better than this PR. The only other thing that I can think of is that making a redis SAVE(http:https://redis.io/commands/save) file that has a data snapshot that gets reloaded after each test. That could work if the redis instance was local to the tests, but wouldn't work if the redis instance is remote. I'm also not sure about the speed of it as redis.

[sjurgemeyer]

Yeah, there are ways we could try to address those issues, but this is starting to feel like going down a rabbit hole for something that may or may not be useful to others. What if I create a PR that makes the wiring change so that we can override the redisService in our project more easily? I could also pull out a couple of the functions currently in GrailsRedisPlugin into a utility class so that it's easier to copy the wiring behavior of the plugin. Then we can create a TestRedisService for our case without creating a specific solution for everyone else.

[tednaleid]

I like that solution Shaun.

I think that there's a feature here that could be useful to others, but if we only go half way with it, it'll end up being more confusing to people who aren't familiar with the limitations. We're essentially trying to do rolled back transactions and that isn't easy to do cleanly.

[sjurgemeyer]

Closing in favor of #46