Test FakeRedis against real Redis #900
Conversation
|
Whoops, I left some tests commented. I'll fix that shortly. |
|
|
||
|
|
||
| # TestFakeRedisVerify.skip = "foo" | ||
| # TestFakeRedisVerifyAsync.skip = "foo" |
There was a problem hiding this comment.
Good catch, thanks. I'll remove them.
|
Would it be worth running the vumi-go tests using the fake redis changes? Not sure if that needs to be done before this PR lands though. |
Good idea, I'll do that now. Thanks! |
|
vumi-go is happy with the new fake Redis. (Most of the changes were response values on write operations, so we were pretty much ignoring them anyway.) |
Awesome, thanks for checking. |
| Scanning returns keys in an order that depends on arbitrary state in | ||
| the Redis server, so we can't fake it in a way that's identical to real | ||
| Redis. | ||
| """ |
There was a problem hiding this comment.
I would've thought testing this against both fake and real redis is possible if we just sort the returned keys before asserting on them (along with a docstring explaining why sorting is necessary in the test). Any reason why that wouldn't work that I'm missing?
There was a problem hiding this comment.
Oh, right, we can't assume that a set of keys returned by a real redis scan call will always be the same set of keys. For some reason I assumed that would be the case, nevermind.
There was a problem hiding this comment.
Yeah. I tried to rewrite the tests in a way that would let us walk the scan over the whole set of keys and check that we got the same set of results at the end, but that ended up being more effort than it's really worth.
|
I might just be not finding it in the diff, I don't think we have tests for |
|
Looks good, left some comments. |
I'd prefer to leave that for another branch -- the purpose of this one is to build the verification infrastructure. I only added new tests for things that changed in ways that required modifications to other tests, like |
Happy for that to happen in a separate PR |
| try: | ||
| value = int(value) + int(str(amount)) | ||
| except (TypeError, ValueError): | ||
| raise ResponseError("value is not an integer or out of range") |
There was a problem hiding this comment.
Do we have tests for these cases:
- using
"0"as the default field hincrbyagainst a key holding the wrong kind of valuehincrbywith a non-integer or out of range value
If not, could we add some?
There was a problem hiding this comment.
Theses cases are all covered in test_hincrby.
|
👍 when travis is happy. Should we make tickets for |
|
I opened #926 for the test coverage improvements. |
There are a number of differences between the behaviours of FakeRedis and a real Redis client+server combination. We really should run the same set of tests for both and fix the differences.