Mock connection error #197

Amertz08 · 2018-07-07T15:50:09Z

In reference to #102

Created a decorator to be added to top level methods that will throw a ConnectionError when connect=False is passed in constructor.

…ead of throwing an error

# Conflicts: # fakeredis.py

coveralls · 2018-07-07T16:16:53Z

Coverage increased (+0.01%) to 98.479% when pulling 28cae30 on Amertz08:102-mock-connection-error into 5bd2f1b on jamesls:master.

bmerry

Thanks, this looks like a reasonable approach. One thing that I think could be improved is to remove the need to put a decorator on every method. The constructor already (optionally) applies a transformation to every method (_patch_responses), so I'd prefer to do something similar (and maybe have a common function to decide whether an instance member is a command, so that it can be extended later).

A metaclass might be the most Pythonic solution and has the advantage that it patches the class rather than every instance, but it may be a bit of a pain because the syntax changes between Python 2 and 3. I'd also be happy if it was just implemented the same way as _patch_responses.

The unit test is certainly thorough, but may be overkill and create more work to maintain as new commands are added. A few different cases (e.g. a simple command, commands that have a few of the existing decorators, a command that involves multiple databases, a transaction-related command etc) should be sufficient. If you prefer to keep it as is I won't object. @jamesls do you have an opinion here?

It might also be nice to have a public interface to toggle the _connected flag (which could just be removing the _ to make the flag public) so that people can write tests similar to the one you wrote for test_rename.

bmerry · 2018-07-10T07:54:03Z

fakeredis.py

@@ -331,6 +331,17 @@ def release(self):
        self.redis.delete(self.name)


+def check_conn(func):


I see this as an internal function, so should have an underscore prefix.

Amertz08 · 2018-07-10T11:17:13Z

@bmerry Yeah I realized working through it that a decorator on every function was becoming a mentally laborious and I had wished there there was a sole underlying function that everything called so I could simply put the logic in it like the actual Redis implementation. I had not noticed the technique used for _patch_response and that does look like it might be the best solution to reduce the usage of the decorator.

Amertz08 · 2018-07-10T11:27:56Z

If we go the _patch_responses route may I suggest the following modification.

# Pass in the decorator
def _patch_responses(obj, decorator):
    for attr_name in dir(obj):
        attr = getattr(obj, attr_name)
        if not callable(attr) or attr_name.startswith('_'):
            continue
        func = decorator(attr)
        setattr(obj, attr_name, func)


def _check_conn(func):
    """Used to mock connection errors"""
    @functools.wraps(func)
    def func_wrapper(*args, **kwargs):

        if not func.__self__.connected:
            raise redis.ConnectionError
        return func(*args, **kwargs)
    return func_wrapper


class FakeStrictRedis(object):
    # deleted for brevity
    def __init__(self, db=0, charset='utf-8', errors='strict',
                 decode_responses=False, singleton=True, connected=True, **kwargs):
        # deleted for brevity
        self._decode_responses = decode_responses
        self.connected = connected
        if decode_responses:
            _patch_responses(self, _make_decode_func)
        if not connected:
            _patch_responses(self, _check_conn)

bmerry · 2018-07-10T12:16:31Z

Sure, that sounds sensible.

…rough _patch_responses

Amertz08 · 2018-07-10T23:25:27Z

Also think you are right on the testing but I wanted to at least make sure I had them all covered which was nice to have in order to test after removing the decorator on every method. We could, and I'm not sure this is a terribly great testing practice, call all the functions through introspection like how _patch_responses works but we'd likely have to print the method out on failure in order to know which one we are actually calling. Also not entirely sure how'd we go about determining the parameters to pass to the methods outside of creating a dictionary where the keys are the method name and the value is a list of the parameters. Which would still require updates for each time a method is added and is kinda convoluted. Just brainstorming.

def test_simple_calls(self):
    params = {
        'set': ['name', 2]
    }
    for attr_name in dir(self.redis):
        attr = getattr(self.redis, attr_name)
        if not callable(attr) or attr_name.startswith('_'):
            continue
        with self.assertRaises(redis.ConnectionError):
            attr(*params[attr_name])

bmerry · 2018-07-11T11:21:55Z

Okay, we can leave the tests as is for now - trying to do it by introspection sounds yucky. We can always remove them later if they become annoying to maintain.

Can you perhaps write a section in the README to describe this feature?

bmerry · 2018-07-11T11:18:12Z

fakeredis.py

-            _patch_responses(self)
+            _patch_responses(self, _make_decode_func)
+
+        if not connected:


Should this be done unconditionally? Otherwise you get the odd behaviour that you can toggle connected on and off, but only if it was initially set to false.

Yes I believe you are correct. I'll implement that change.

bmerry · 2018-07-11T11:20:29Z

fakeredis.py

    @functools.wraps(func)
    def func_wrapper(*args, **kwargs):
-        if not args[0]._connected:
+        if not func.__self__.connected:
            raise redis.ConnectionError


Can you also pass a message with the ConnectionError e.g. "fakeredis is emulating a connection error".

Will do. Kinda been thinking that needed a message.

…f initial state of connection

Amertz08 · 2018-07-11T16:25:20Z

I'm not entirely sure why but it doesn't appear the README is properly rendering the additions I've made. I can see the short text paragraph I wrote but not the code example.

Amertz08 · 2018-07-11T21:42:44Z

@bmerry Nix that question on the documentation. I was able to resolve it.

bmerry · 2018-07-12T06:06:35Z

Thanks for all the updates. I'm going to be away for about a week, so I'll take a fresh look when I'm back.

bmerry

This is looking good. The one question I have is about how FakePubSub should work e.g. whether it should also have a connected attribute and fake connection errors when calling functions like psubscribe. I haven't experimented to see what happens in redis-py in this case.

Amertz08 · 2018-07-18T16:16:06Z

Yeah I'll have to look over redis-py and see how it is implemented and get that in if needed.

Amertz08 · 2018-07-18T21:17:38Z

Here is the underlying function on the PubSub class. Basically the same implementation as the standard Redis class. So maybe we implement it in the same manner as we did with FakeStrictRedis

bmerry · 2018-07-20T10:01:03Z

Here is the underlying function on the PubSub class. Basically the same implementation as the standard Redis class. So maybe we implement it in the same manner as we did with FakeStrictRedis

Sounds reasonable to me.

bmerry · 2018-07-27T07:52:48Z

fakeredis.py

@@ -2231,11 +2231,13 @@ class FakePubSub(object):
    PATTERN_MESSAGE_TYPES = ['psubscribe', 'punsubscribe']
    LISTEN_DELAY = 0.1          # delay between listen loops (seconds)

-    def __init__(self, decode_responses=False, *args, **kwargs):
+    def __init__(self, connected=True, decode_responses=False, *args, **kwargs):


I would rather put connected after decode_responses, so that it doesn't break existing code that passes decode_responses as a positional parameter.

bmerry · 2018-07-27T07:53:46Z

test_fakeredis.py

+class TestPubSubConnected(unittest.TestCase):
+
+    def setUp(self):
+        self.redis = fakeredis.FakePubSub(connected=False)


I would rather call this variable self.pubsub since it is not an instance of a Redis object.

bmerry · 2018-07-27T07:53:59Z

test_fakeredis.py

@@ -4403,5 +4403,42 @@ def test_hscan_iter(self):
            self.redis.hscan_iter('name')


+class TestPubSubConnected(unittest.TestCase):
+


For consistency with other classes, remove this blank line.

bmerry

Thanks for the work on this! I'm going to merge it.

Amertz08 · 2018-07-27T09:33:55Z

Alright sweet. First OSS contrib.

Amertz08 added 11 commits January 10, 2018 14:39

Started implementing connection error throws

bb70bcd

updated test cases

6aea29c

Updated ConnectionError test cases

5359bdf

Updated test case to actually fail when decorator does not exist inst…

f940176

…ead of throwing an error

Merge branch 'master' into 102-mock-connection-error

17a98dd

# Conflicts: # fakeredis.py

Added venv to ignore file

ffb6205

Added missing check_conn so tests work

4263daa

Updated tests and added check decorator as needed

2164cb6

finished remaining tests

cbaba98

flake8

ef97612

Fixed issue with decorator not playing well with _lua_reply

269fc1c

bmerry requested changes Jul 10, 2018

View reviewed changes

Amertz08 added 5 commits July 10, 2018 14:46

Modified _patch_responses to take the decorator as a parameter

49db0a3

Made connection check decorator private function. Adjusted for use th…

5dccd2a

…rough _patch_responses

Removed decorator from methods

c1a80b3

Adjusted patch_responses call for FakePubSub class

fa63151

Adjusted test cases for access to connected attribute

0fa4e3c

bmerry requested changes Jul 11, 2018

View reviewed changes

Amertz08 added 4 commits July 11, 2018 11:12

Connection error decorator should be patched to response regardless o…

ed56f4a

…f initial state of connection

ConnectionError message

dc3e59e

Updated README with documentation on mocking connection errors

bfe919b

Proper indentation in README

03615de

_check_conn should be patched on before decode_responses

71e89ba

Spacing fix on documentation

8412e25

bmerry reviewed Jul 18, 2018

View reviewed changes

Patched connection error decorator to PubSub class. Added test cases

6f301b3

bmerry requested changes Jul 27, 2018

View reviewed changes

Amertz08 added 2 commits July 27, 2018 03:14

Connected arg after decode arg in FakePubSub

75dced3

Renamed test attribute redis to pubsub

28cae30

bmerry approved these changes Jul 27, 2018

View reviewed changes

bmerry merged commit 70d4910 into jamesls:master Jul 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mock connection error #197

Mock connection error #197

Amertz08 commented Jul 7, 2018

coveralls commented Jul 7, 2018 •

edited

bmerry left a comment

bmerry Jul 10, 2018 •

edited

Amertz08 commented Jul 10, 2018

Amertz08 commented Jul 10, 2018 •

edited

bmerry commented Jul 10, 2018

Amertz08 commented Jul 10, 2018 •

edited

bmerry commented Jul 11, 2018

bmerry Jul 11, 2018

Amertz08 Jul 11, 2018

bmerry Jul 11, 2018

Amertz08 Jul 11, 2018

Amertz08 commented Jul 11, 2018

Amertz08 commented Jul 11, 2018

bmerry commented Jul 12, 2018

bmerry left a comment

Amertz08 commented Jul 18, 2018

Amertz08 commented Jul 18, 2018 •

edited

bmerry commented Jul 20, 2018

bmerry Jul 27, 2018

bmerry Jul 27, 2018

bmerry Jul 27, 2018

bmerry left a comment

Amertz08 commented Jul 27, 2018

		@@ -331,6 +331,17 @@ def release(self):
		self.redis.delete(self.name)


		def check_conn(func):

		@@ -4403,5 +4403,42 @@ def test_hscan_iter(self):
		self.redis.hscan_iter('name')


		class TestPubSubConnected(unittest.TestCase):

Mock connection error #197

Mock connection error #197

Conversation

Amertz08 commented Jul 7, 2018

coveralls commented Jul 7, 2018 • edited

bmerry left a comment

Choose a reason for hiding this comment

bmerry Jul 10, 2018 • edited

Choose a reason for hiding this comment

Amertz08 commented Jul 10, 2018

Amertz08 commented Jul 10, 2018 • edited

bmerry commented Jul 10, 2018

Amertz08 commented Jul 10, 2018 • edited

bmerry commented Jul 11, 2018

bmerry Jul 11, 2018

Choose a reason for hiding this comment

Amertz08 Jul 11, 2018

Choose a reason for hiding this comment

bmerry Jul 11, 2018

Choose a reason for hiding this comment

Amertz08 Jul 11, 2018

Choose a reason for hiding this comment

Amertz08 commented Jul 11, 2018

Amertz08 commented Jul 11, 2018

bmerry commented Jul 12, 2018

bmerry left a comment

Choose a reason for hiding this comment

Amertz08 commented Jul 18, 2018

Amertz08 commented Jul 18, 2018 • edited

bmerry commented Jul 20, 2018

bmerry Jul 27, 2018

Choose a reason for hiding this comment

bmerry Jul 27, 2018

Choose a reason for hiding this comment

bmerry Jul 27, 2018

Choose a reason for hiding this comment

bmerry left a comment

Choose a reason for hiding this comment

Amertz08 commented Jul 27, 2018

coveralls commented Jul 7, 2018 •

edited

bmerry Jul 10, 2018 •

edited

Amertz08 commented Jul 10, 2018 •

edited

Amertz08 commented Jul 10, 2018 •

edited

Amertz08 commented Jul 18, 2018 •

edited