-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Align FakeConnection
constructor signature to base class
#293
Conversation
My actual workaround to use fakeredis to mock a Redis instance used as a cache backend via django-redis library in a Django project is the following: import django.core.cache
from django.test import TestCase, override_settings
import fakeredis
import mock
_caches = {
'default': {
'BACKEND': 'django_redis.cache.RedisCache',
'LOCATION': 'redis://fake-redis/0',
'OPTIONS': {
'CLIENT_CLASS': 'django_redis.client.DefaultClient',
}
}
}
@override_settings(CACHES=_caches)
@mock.patch("django_redis.pool.ConnectionFactory.get_connection",
new=mock.MagicMock(return_value=fakeredis.FakeRedis()))
class TestCache(TestCase):
def test_set_unicode_key_redis_cache(self):
try:
django.core.cache.caches['default'].set('unicode-key-€', 'fake_value')
except UnicodeEncodeError:
self.fail("`RedisCache.set` raised "
"UnicodeEncodeError unexpectedly") |
Thanks. This seems reasonable, and I think using the base class Travis is showing a flake8 error that is preventing the rest of the unit tests from running. I think what is likely to happen is that it will fail on older versions of redis-py that didn't have all the arguments that the current Connection class takes. It may be possible to fix that by having the constructor take |
ed48dcf
to
c17aac8
Compare
@bmerry Thanks to you for your quick reply. I checked all the Travis CI builds and indeed the flake8 commands failed, but apparently for another reason:
This is caused by the fact that I removed some code in the I removed that unused import. Let's see. |
It looks like there are now some failures of the sort I expected, but also these:
That seems to be some bug on master that for some reason hasn't appeared before, so don't worry about those. |
You're right. Tests actually fails for redis-py versions lower than 3.3.11. I followed your suggestion to use variadic arguments. I think it's a safer way to guarantee compatibility with multiple redis-py interfaces. |
c17aac8
to
a76a0f3
Compare
@bmerry Excluding the test you mentioned, now only one the CI job using redis-py 2.10.6 fails. Precisely, only the following test fails:
At first glance, it's clear that the purpose of that test is to ensure the If I use fakeredis in a project where I use redis-py 2.10.6, I should be aware I cannot instantiate a |
I've fixed up master. That test is intended to ensure that arguments that can be passed to redis can also be passed to fakeredis, even though it doesn't use them. I think the simplest thing is just to add the |
As `FakeConnection` is a subclass of the `Connection` class in `redis-py`, in order to honor OOP substitutability principle, the signature of the constructor should be aligned with the one of the base class. That means it should contain at least all the parameters of the base class. This automatically fixes a problem occurring when the connection object is dynamically instantiated within the context of the default connection pool class in `redis-py`, named `ConnectionPool`, and when, in turn, the latter is dynamically instantiated using its standard `from_url` factory method. In such scenario the instantiation of the connection object fails because the `FakeConnection` class misses both `host` and `port` parameters in its constructor signature. That happens because the `from_url` factory method above, given a canonical Redis URL, guesses several connection parameters values, among which `host`, `username`, `password`, `path`, and `db`, which values override any other corresponding value passed to the factory method itself via the connection variadic arguments. Aligning the `FakeConnection` constructor signature to the one of its base class, by using variadic arguments, is helpful in contexts where there's no direct control over the instantiantiation of Redis clients, connections and connection pools objects (e.g. `django-redis` library): whilst preserving current behavior, this avoids to workaround the issue by defining either custom connection factories or custom connection pools.
a76a0f3
to
f8b2170
Compare
That makes sense. |
In future please don't rewrite history on a PR. It makes it impossible to see what changed since my previous review. |
Sorry, I won't do it again. I did because sometimes I found out that Travis CI do not automatically run Pull Request builds. |
Thanks for the contribution! Since you mentioned that this is for use with django,-redis, which seems to be a popular usage of fakeredis but not one I'm familiar with, would you be interested in making another PR to add instructions to the README? |
I sure can.
…On Mon, Mar 15, 2021, 8:46 AM Bruce Merry ***@***.***> wrote:
Thanks for the contribution! Since you mentioned that this is for use with
django,-redis, which seems to be a popular usage of fakeredis but not one
I'm familiar with, would you be interested in making another PR to add
instructions to the README?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#293 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF4JFLSMHNCHDK5RVME5NELTDW3N5ANCNFSM4XAWQGVA>
.
|
As
FakeConnection
is a subclass of theConnection
class inredis-py
, in order to honor OOP substitutability principle, the signature of the constructor should be aligned with the one of the base class. That means it should contain at least all the parameters of the base class.This automatically fixes a problem occurring when the connection object is dynamically instantiated within the context of the default connection pool class in
redis-py
, namedConnectionPool
, and when, in turn, the latter is dynamically instantiated using its standardfrom_url
factory method. In such scenario the instantiation of the connection object fails because theFakeConnection
class misses bothhost
andport
parameters in its constructor signature.That happens because the
from_url
factory method above, given a canonical Redis URL, guesses several connection parameters values, among whichhost
,username
,password
,path
, anddb
, which values override any other corresponding value passed to the factory method itself via the connection variadic arguments.Aligning the
FakeConnection
constructor signature to the one of its base class, by including thehost
andport
parameters, is helpful in contexts where there's no direct control over the instantiantiation of Redis clients, connections and connection pools objects (e.g.django-redis
library): whilst preserving current behavior, this avoids to workaround the issue by defining either custom connection factories or custom connection pools.Closes #234