Support aioredis 2 #304
Support aioredis 2 #304
Conversation
Some of the basics work, but there are still several failing tests (some are tests that need updating, but at a minimum pubsub appears not to be working).
| pool.connection_kwargs['server'] = server | ||
| for key in ['password', 'host', 'port', 'path']: | ||
| if key in pool.connection_kwargs: | ||
| del pool.connection_kwargs[key] |
There was a problem hiding this comment.
This wasn't really related, but it's no longer necessary now that FakeConnection can absorb arbitrary kwargs.
|
|
||
|
|
||
| def test_hmsetset_empty_raises_error(r): | ||
| def test_hmset_empty_raises_error(r): |
There was a problem hiding this comment.
Changes in this file aren't really related - just a typo/search-replace error that I spotted.
Waiting for jamesls/fakeredis#304 to release to finish making the tests work
Waiting for jamesls/fakeredis#304 to release to finish making the tests work
| aioredis2 = aioredis.__version__ >= distutils.version.StrictVersion('2.0.0a1') | ||
| pytestmark = [ | ||
| pytest.mark.asyncio, | ||
| pytest.mark.skipif(not aioredis2, reason="Test is only applicable to aioredis 2.x") |
There was a problem hiding this comment.
| pytest.mark.skipif(not aioredis2, reason="Test is only applicable to aioredis 2.x") | |
| pytest.mark.skipif(not aioredis2, reason="Test is only applicable to aioredis 1.x") |
There was a problem hiding this comment.
The unit tests in test_aioredis2.py are presumably meant for aioredis 2.x, so I think the original sense is correct.
There was a problem hiding this comment.
A test is being skipped here if it's not marked as being for aioredis 2.x so I think the reason should be what I've proposed? Or am I missing something?
There was a problem hiding this comment.
The way I read it is that not aioredis2 means that aioredis 1 is installed. Then all tests in test_aioredis2.py will be skipped, because they are meant to be run with aioredis 2.x.
A similar (but inverse) pattern is used in test_aioredis1.py.
There was a problem hiding this comment.
I think the confusion on my side came from the fact that I thought that in the 1.x tests the message was also talking about 2.x
There was a problem hiding this comment.
No worries - I first had to figure out that pytestmark actually does something... Ah, the magic of pytest 😂
ludwigschwardt
left a comment
ludwigschwardt
left a comment
There was a problem hiding this comment.
Not an in-depth review, but a quick scan and crosscheck with v1.
|
|
||
|
|
||
| class FakeReader: | ||
| pass |
There was a problem hiding this comment.
Out of curiosity, why don't you need a Reader with aioredis 2?
There was a problem hiding this comment.
fakeredis is a bit asymmetric (and one day I might try to address that): commands get serialised by aioredis/redis-py and parsed by fakeredis, but responses are kept as structured Python objects and the response parsing is bypassed.
| import fakeredis.aioredis | ||
|
|
||
|
|
||
| aioredis2 = aioredis.__version__ >= distutils.version.StrictVersion('2.0.0a1') |
There was a problem hiding this comment.
Have you considered packaging or is it not worth the extra install?
There was a problem hiding this comment.
Actually I see distutils is going to be deprecated in Python 3.10 and packaging will be recommended. So I might as well make the change now.
There was a problem hiding this comment.
Although I'll make a separate PR so as not to hold up this release.
Adds support for aioredis 2. Since this is a complete rewrite with a different API, there are different backend modules for each, and the appropriate one is imported.
_aioredis1.pyis essentially the oldaioredis.py, minus some bits of common code that were refactored intoasync.py.