From bf6c113319d47594e103c71f8ff5b0ea48d15b38 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 17 May 2020 03:18:19 +0200 Subject: Test suite for the redis dict. --- tests/bot/utils/test_redis_dict.py | 189 +++++++++++++++++++++++++++++++++++++ 1 file changed, 189 insertions(+) create mode 100644 tests/bot/utils/test_redis_dict.py (limited to 'tests') diff --git a/tests/bot/utils/test_redis_dict.py b/tests/bot/utils/test_redis_dict.py new file mode 100644 index 000000000..f422887ce --- /dev/null +++ b/tests/bot/utils/test_redis_dict.py @@ -0,0 +1,189 @@ +import unittest + +import fakeredis +from redis import DataError + +from bot.utils import RedisDict + +redis_server = fakeredis.FakeServer() +RedisDict._redis = fakeredis.FakeStrictRedis(server=redis_server) + + +class RedisDictTests(unittest.TestCase): + """Tests the RedisDict class from utils.redis_dict.py.""" + + redis = RedisDict() + + def test_class_attribute_namespace(self): + """Test that RedisDict creates a namespace automatically for class attributes.""" + self.assertEqual(self.redis._namespace, "RedisDictTests.redis") + + def test_custom_namespace(self): + """Test that users can set a custom namespaces which never collide.""" + test_cases = ( + (RedisDict("firedog")._namespace, "firedog"), + (RedisDict("firedog")._namespace, "firedog_"), + (RedisDict("firedog")._namespace, "firedog__"), + ) + + for test_case, result in test_cases: + self.assertEqual(test_case, result) + + def test_custom_namespace_takes_precedence(self): + """Test that custom namespaces take precedence over class attribute ones.""" + class LemonJuice: + citrus = RedisDict("citrus") + watercat = RedisDict() + + test_class = LemonJuice() + self.assertEqual(test_class.citrus._namespace, "citrus") + self.assertEqual(test_class.watercat._namespace, "LemonJuice.watercat") + + def test_set_get_item(self): + """Test that users can set and get items from the RedisDict.""" + self.redis['favorite_fruit'] = 'melon' + self.redis['favorite_number'] = 86 + self.assertEqual(self.redis['favorite_fruit'], 'melon') + self.assertEqual(self.redis['favorite_number'], 86) + + def test_set_item_value_types(self): + """Test that setitem rejects values that are not JSON serializable.""" + with self.assertRaises(TypeError): + self.redis['favorite_thing'] = object + self.redis['favorite_stuff'] = RedisDict + + def test_set_item_key_types(self): + """Test that setitem rejects keys that are not strings, ints or floats.""" + fruits = ["lemon", "melon", "apple"] + + with self.assertRaises(DataError): + self.redis[fruits] = "nice" + + def test_get_method(self): + """Test that the .get method works like in a dict.""" + self.redis['favorite_movie'] = 'Code Jam Highlights' + + self.assertEqual(self.redis.get('favorite_movie'), 'Code Jam Highlights') + self.assertEqual(self.redis.get('favorite_youtuber', 'pydis'), 'pydis') + self.assertIsNone(self.redis.get('favorite_dog')) + + def test_membership(self): + """Test that we can reliably use the `in` operator with our RedisDict.""" + self.redis['favorite_country'] = "Burkina Faso" + + self.assertIn('favorite_country', self.redis) + self.assertNotIn('favorite_dentist', self.redis) + + def test_del_item(self): + """Test that users can delete items from the RedisDict.""" + self.redis['favorite_band'] = "Radiohead" + self.assertIn('favorite_band', self.redis) + + del self.redis['favorite_band'] + self.assertNotIn('favorite_band', self.redis) + + def test_iter(self): + """Test that the RedisDict can be iterated.""" + self.redis.clear() + test_cases = ( + ('favorite_turtle', 'Donatello'), + ('second_favorite_turtle', 'Leonardo'), + ('third_favorite_turtle', 'Raphael'), + ) + for key, value in test_cases: + self.redis[key] = value + + # Test regular iteration + for test_case, key in zip(test_cases, self.redis): + value = test_case[1] + self.assertEqual(self.redis[key], value) + + # Test .items iteration + for key, value in self.redis.items(): + self.assertEqual(self.redis[key], value) + + # Test .keys iteration + for test_case, key in zip(test_cases, self.redis.keys()): + value = test_case[1] + self.assertEqual(self.redis[key], value) + + def test_len(self): + """Test that we can get the correct len() from the RedisDict.""" + self.redis.clear() + self.redis['one'] = 1 + self.redis['two'] = 2 + self.redis['three'] = 3 + self.assertEqual(len(self.redis), 3) + + self.redis['four'] = 4 + self.assertEqual(len(self.redis), 4) + + def test_copy(self): + """Test that the .copy method returns a workable dictionary copy.""" + copy = self.redis.copy() + local_copy = dict(self.redis.items()) + self.assertIs(type(copy), dict) + self.assertEqual(copy, local_copy) + + def test_clear(self): + """Test that the .clear method removes the entire hash.""" + self.redis.clear() + self.redis['teddy'] = "with me" + self.redis['in my dreams'] = "you have a weird hat" + self.assertEqual(len(self.redis), 2) + + self.redis.clear() + self.assertEqual(len(self.redis), 0) + + def test_pop(self): + """Test that we can .pop an item from the RedisDict.""" + self.redis.clear() + self.redis['john'] = 'was afraid' + + self.assertEqual(self.redis.pop('john'), 'was afraid') + self.assertEqual(self.redis.pop('pete', 'breakneck'), 'breakneck') + self.assertEqual(len(self.redis), 0) + + def test_popitem(self): + """Test that we can .popitem an item from the RedisDict.""" + self.redis.clear() + self.redis['john'] = 'the revalator' + self.redis['teddy'] = 'big bear' + + self.assertEqual(len(self.redis), 2) + self.assertEqual(self.redis.popitem(), 'big bear') + self.assertEqual(len(self.redis), 1) + + def test_setdefault(self): + """Test that we can .setdefault an item from the RedisDict.""" + self.redis.clear() + self.redis.setdefault('john', 'is yellow and weak') + self.assertEqual(self.redis['john'], 'is yellow and weak') + + with self.assertRaises(TypeError): + self.redis.setdefault('geisha', object) + + def test_update(self): + """Test that we can .update the RedisDict with multiple items.""" + self.redis.clear() + self.redis["reckfried"] = "lona" + self.redis["bel air"] = "prince" + self.redis.update({ + "reckfried": "jona", + "mega": "hungry, though", + }) + + result = { + "reckfried": "jona", + "bel air": "prince", + "mega": "hungry, though", + } + self.assertEqual(self.redis.copy(), result) + + def test_equals(self): + """Test that RedisDicts can be compared with == and !=.""" + new_redis_dict = RedisDict("firedog_the_sequel") + new_new_redis_dict = new_redis_dict + + self.assertEqual(new_redis_dict, new_new_redis_dict) + self.assertNotEqual(new_redis_dict, self.redis) -- cgit v1.2.3 From 57fe4bf893e94289b5b6f7158ff2d6b92b1e3fae Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Fri, 22 May 2020 22:56:25 +0200 Subject: Set up async testbed --- bot/bot.py | 3 +- bot/utils/redis_cache.py | 13 ++- tests/bot/utils/test_redis_cache.py | 128 ++++++++++++++++++++++++ tests/bot/utils/test_redis_dict.py | 189 ------------------------------------ 4 files changed, 135 insertions(+), 198 deletions(-) create mode 100644 tests/bot/utils/test_redis_cache.py delete mode 100644 tests/bot/utils/test_redis_dict.py (limited to 'tests') diff --git a/bot/bot.py b/bot/bot.py index f55eec5bb..8a3805989 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -45,8 +45,7 @@ class Bot(commands.Bot): # will effectively disable stats. statsd_url = "127.0.0.1" - asyncio.create_task(self._create_redis_session()) - + self.loop.create_task(self._create_redis_session()) self.stats = AsyncStatsClient(self.loop, statsd_url, 8125, prefix="bot") async def _create_redis_session(self) -> None: diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 467f16767..483bbc2cd 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -30,8 +30,8 @@ class RedisCache: def __init__(self) -> None: """Raise a NotImplementedError if `__set_name__` hasn't been run.""" - if not self._namespace: - raise NotImplementedError("RedisCache must be a class attribute.") + self._namespace = None + self.bot = None def _set_namespace(self, namespace: str) -> None: """Try to set the namespace, but do not permit collisions.""" @@ -47,8 +47,7 @@ class RedisCache: Called automatically when this class is constructed inside a class as an attribute. """ - if not self._has_custom_namespace: - self._set_namespace(f"{owner.__name__}.{attribute_name}") + self._set_namespace(f"{owner.__name__}.{attribute_name}") def __get__(self, instance: RedisCache, owner: Any) -> RedisCache: """Fetch the Bot instance, we need it for the redis pool.""" @@ -106,9 +105,9 @@ class RedisCache: async def pop(self, key: ValidRedisKey, default: Optional[JSONSerializableType] = None) -> JSONSerializableType: """Get the item, remove it from the cache, and provide a default if not found.""" - value = await self.get(key, default) - await self.delete(key) - return value + # value = await self.get(key, default) + # await self.delete(key) + # return value async def update(self) -> None: """Update the Redis cache with multiple values.""" diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py new file mode 100644 index 000000000..f6344803f --- /dev/null +++ b/tests/bot/utils/test_redis_cache.py @@ -0,0 +1,128 @@ +import asyncio +import unittest +from unittest.mock import MagicMock + +import fakeredis.aioredis + +from bot.bot import Bot +from bot.utils import RedisCache + + +class RedisCacheTests(unittest.IsolatedAsyncioTestCase): + """Tests the RedisDict class from utils.redis_dict.py.""" + + redis = RedisCache() + + async def asyncSetUp(self): # noqa: N802 - this special method can't be all lowercase + """Sets up the objects that only have to be initialized once.""" + self.bot = MagicMock( + spec=Bot, + redis_session=await fakeredis.aioredis.create_redis_pool(), + _redis_ready=asyncio.Event(), + ) + self.bot._redis_ready.set() + + def test_class_attribute_namespace(self): + """Test that RedisDict creates a namespace automatically for class attributes.""" + self.assertEqual(self.redis._namespace, "RedisCacheTests.redis") + # Test that errors are raised when this isn't true. + + # def test_set_get_item(self): + # """Test that users can set and get items from the RedisDict.""" + # self.redis['favorite_fruit'] = 'melon' + # self.redis['favorite_number'] = 86 + # self.assertEqual(self.redis['favorite_fruit'], 'melon') + # self.assertEqual(self.redis['favorite_number'], 86) + # + # def test_set_item_types(self): + # """Test that setitem rejects keys and values that are not strings, ints or floats.""" + # fruits = ["lemon", "melon", "apple"] + # + # with self.assertRaises(DataError): + # self.redis[fruits] = "nice" + # + # def test_contains(self): + # """Test that we can reliably use the `in` operator with our RedisDict.""" + # self.redis['favorite_country'] = "Burkina Faso" + # + # self.assertIn('favorite_country', self.redis) + # self.assertNotIn('favorite_dentist', self.redis) + # + # def test_items(self): + # """Test that the RedisDict can be iterated.""" + # self.redis.clear() + # test_cases = ( + # ('favorite_turtle', 'Donatello'), + # ('second_favorite_turtle', 'Leonardo'), + # ('third_favorite_turtle', 'Raphael'), + # ) + # for key, value in test_cases: + # self.redis[key] = value + # + # # Test regular iteration + # for test_case, key in zip(test_cases, self.redis): + # value = test_case[1] + # self.assertEqual(self.redis[key], value) + # + # # Test .items iteration + # for key, value in self.redis.items(): + # self.assertEqual(self.redis[key], value) + # + # # Test .keys iteration + # for test_case, key in zip(test_cases, self.redis.keys()): + # value = test_case[1] + # self.assertEqual(self.redis[key], value) + # + # def test_length(self): + # """Test that we can get the correct len() from the RedisDict.""" + # self.redis.clear() + # self.redis['one'] = 1 + # self.redis['two'] = 2 + # self.redis['three'] = 3 + # self.assertEqual(len(self.redis), 3) + # + # self.redis['four'] = 4 + # self.assertEqual(len(self.redis), 4) + # + # def test_to_dict(self): + # """Test that the .copy method returns a workable dictionary copy.""" + # copy = self.redis.copy() + # local_copy = dict(self.redis.items()) + # self.assertIs(type(copy), dict) + # self.assertEqual(copy, local_copy) + # + # def test_clear(self): + # """Test that the .clear method removes the entire hash.""" + # self.redis.clear() + # self.redis['teddy'] = "with me" + # self.redis['in my dreams'] = "you have a weird hat" + # self.assertEqual(len(self.redis), 2) + # + # self.redis.clear() + # self.assertEqual(len(self.redis), 0) + # + # def test_pop(self): + # """Test that we can .pop an item from the RedisDict.""" + # self.redis.clear() + # self.redis['john'] = 'was afraid' + # + # self.assertEqual(self.redis.pop('john'), 'was afraid') + # self.assertEqual(self.redis.pop('pete', 'breakneck'), 'breakneck') + # self.assertEqual(len(self.redis), 0) + # + # def test_update(self): + # """Test that we can .update the RedisDict with multiple items.""" + # self.redis.clear() + # self.redis["reckfried"] = "lona" + # self.redis["bel air"] = "prince" + # self.redis.update({ + # "reckfried": "jona", + # "mega": "hungry, though", + # }) + # + # result = { + # "reckfried": "jona", + # "bel air": "prince", + # "mega": "hungry, though", + # } + # self.assertEqual(self.redis.copy(), result) diff --git a/tests/bot/utils/test_redis_dict.py b/tests/bot/utils/test_redis_dict.py deleted file mode 100644 index f422887ce..000000000 --- a/tests/bot/utils/test_redis_dict.py +++ /dev/null @@ -1,189 +0,0 @@ -import unittest - -import fakeredis -from redis import DataError - -from bot.utils import RedisDict - -redis_server = fakeredis.FakeServer() -RedisDict._redis = fakeredis.FakeStrictRedis(server=redis_server) - - -class RedisDictTests(unittest.TestCase): - """Tests the RedisDict class from utils.redis_dict.py.""" - - redis = RedisDict() - - def test_class_attribute_namespace(self): - """Test that RedisDict creates a namespace automatically for class attributes.""" - self.assertEqual(self.redis._namespace, "RedisDictTests.redis") - - def test_custom_namespace(self): - """Test that users can set a custom namespaces which never collide.""" - test_cases = ( - (RedisDict("firedog")._namespace, "firedog"), - (RedisDict("firedog")._namespace, "firedog_"), - (RedisDict("firedog")._namespace, "firedog__"), - ) - - for test_case, result in test_cases: - self.assertEqual(test_case, result) - - def test_custom_namespace_takes_precedence(self): - """Test that custom namespaces take precedence over class attribute ones.""" - class LemonJuice: - citrus = RedisDict("citrus") - watercat = RedisDict() - - test_class = LemonJuice() - self.assertEqual(test_class.citrus._namespace, "citrus") - self.assertEqual(test_class.watercat._namespace, "LemonJuice.watercat") - - def test_set_get_item(self): - """Test that users can set and get items from the RedisDict.""" - self.redis['favorite_fruit'] = 'melon' - self.redis['favorite_number'] = 86 - self.assertEqual(self.redis['favorite_fruit'], 'melon') - self.assertEqual(self.redis['favorite_number'], 86) - - def test_set_item_value_types(self): - """Test that setitem rejects values that are not JSON serializable.""" - with self.assertRaises(TypeError): - self.redis['favorite_thing'] = object - self.redis['favorite_stuff'] = RedisDict - - def test_set_item_key_types(self): - """Test that setitem rejects keys that are not strings, ints or floats.""" - fruits = ["lemon", "melon", "apple"] - - with self.assertRaises(DataError): - self.redis[fruits] = "nice" - - def test_get_method(self): - """Test that the .get method works like in a dict.""" - self.redis['favorite_movie'] = 'Code Jam Highlights' - - self.assertEqual(self.redis.get('favorite_movie'), 'Code Jam Highlights') - self.assertEqual(self.redis.get('favorite_youtuber', 'pydis'), 'pydis') - self.assertIsNone(self.redis.get('favorite_dog')) - - def test_membership(self): - """Test that we can reliably use the `in` operator with our RedisDict.""" - self.redis['favorite_country'] = "Burkina Faso" - - self.assertIn('favorite_country', self.redis) - self.assertNotIn('favorite_dentist', self.redis) - - def test_del_item(self): - """Test that users can delete items from the RedisDict.""" - self.redis['favorite_band'] = "Radiohead" - self.assertIn('favorite_band', self.redis) - - del self.redis['favorite_band'] - self.assertNotIn('favorite_band', self.redis) - - def test_iter(self): - """Test that the RedisDict can be iterated.""" - self.redis.clear() - test_cases = ( - ('favorite_turtle', 'Donatello'), - ('second_favorite_turtle', 'Leonardo'), - ('third_favorite_turtle', 'Raphael'), - ) - for key, value in test_cases: - self.redis[key] = value - - # Test regular iteration - for test_case, key in zip(test_cases, self.redis): - value = test_case[1] - self.assertEqual(self.redis[key], value) - - # Test .items iteration - for key, value in self.redis.items(): - self.assertEqual(self.redis[key], value) - - # Test .keys iteration - for test_case, key in zip(test_cases, self.redis.keys()): - value = test_case[1] - self.assertEqual(self.redis[key], value) - - def test_len(self): - """Test that we can get the correct len() from the RedisDict.""" - self.redis.clear() - self.redis['one'] = 1 - self.redis['two'] = 2 - self.redis['three'] = 3 - self.assertEqual(len(self.redis), 3) - - self.redis['four'] = 4 - self.assertEqual(len(self.redis), 4) - - def test_copy(self): - """Test that the .copy method returns a workable dictionary copy.""" - copy = self.redis.copy() - local_copy = dict(self.redis.items()) - self.assertIs(type(copy), dict) - self.assertEqual(copy, local_copy) - - def test_clear(self): - """Test that the .clear method removes the entire hash.""" - self.redis.clear() - self.redis['teddy'] = "with me" - self.redis['in my dreams'] = "you have a weird hat" - self.assertEqual(len(self.redis), 2) - - self.redis.clear() - self.assertEqual(len(self.redis), 0) - - def test_pop(self): - """Test that we can .pop an item from the RedisDict.""" - self.redis.clear() - self.redis['john'] = 'was afraid' - - self.assertEqual(self.redis.pop('john'), 'was afraid') - self.assertEqual(self.redis.pop('pete', 'breakneck'), 'breakneck') - self.assertEqual(len(self.redis), 0) - - def test_popitem(self): - """Test that we can .popitem an item from the RedisDict.""" - self.redis.clear() - self.redis['john'] = 'the revalator' - self.redis['teddy'] = 'big bear' - - self.assertEqual(len(self.redis), 2) - self.assertEqual(self.redis.popitem(), 'big bear') - self.assertEqual(len(self.redis), 1) - - def test_setdefault(self): - """Test that we can .setdefault an item from the RedisDict.""" - self.redis.clear() - self.redis.setdefault('john', 'is yellow and weak') - self.assertEqual(self.redis['john'], 'is yellow and weak') - - with self.assertRaises(TypeError): - self.redis.setdefault('geisha', object) - - def test_update(self): - """Test that we can .update the RedisDict with multiple items.""" - self.redis.clear() - self.redis["reckfried"] = "lona" - self.redis["bel air"] = "prince" - self.redis.update({ - "reckfried": "jona", - "mega": "hungry, though", - }) - - result = { - "reckfried": "jona", - "bel air": "prince", - "mega": "hungry, though", - } - self.assertEqual(self.redis.copy(), result) - - def test_equals(self): - """Test that RedisDicts can be compared with == and !=.""" - new_redis_dict = RedisDict("firedog_the_sequel") - new_new_redis_dict = new_redis_dict - - self.assertEqual(new_redis_dict, new_new_redis_dict) - self.assertNotEqual(new_redis_dict, self.redis) -- cgit v1.2.3 From fd6f3d30b4c67f9a81346bb142d4696948fa2812 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Fri, 22 May 2020 15:40:50 -0700 Subject: Fix assertion for `create_task` in duck pond tests The assertion wasn't using the assertion method. Furthermore, it was testing a non-existent function `create_loop` rather than `create_task`. --- tests/bot/cogs/test_duck_pond.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_duck_pond.py b/tests/bot/cogs/test_duck_pond.py index 7e6bfc748..a8c0107c6 100644 --- a/tests/bot/cogs/test_duck_pond.py +++ b/tests/bot/cogs/test_duck_pond.py @@ -45,7 +45,7 @@ class DuckPondTests(base.LoggingTestsMixin, unittest.IsolatedAsyncioTestCase): self.assertEqual(cog.bot, bot) self.assertEqual(cog.webhook_id, constants.Webhooks.duck_pond) - bot.loop.create_loop.called_once_with(cog.fetch_webhook()) + bot.loop.create_task.assert_called_once_with(cog.fetch_webhook()) def test_fetch_webhook_succeeds_without_connectivity_issues(self): """The `fetch_webhook` method waits until `READY` event and sets the `webhook` attribute.""" -- cgit v1.2.3 From 45e6f8dba869a367b01d99a596bd3355802d1fbe Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Fri, 22 May 2020 15:44:04 -0700 Subject: Improve aiohttp context manager mocking in snekbox tests I'm not sure how it even managed to work before. It was calling the `post` coroutine (without specifying a URL) and then changing `__aenter__`. Now, a separate mock is created for the context manager and the `post` simply returns that mocked context manager. --- tests/bot/cogs/test_snekbox.py | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_snekbox.py b/tests/bot/cogs/test_snekbox.py index 1dec0ccaf..84b273a7d 100644 --- a/tests/bot/cogs/test_snekbox.py +++ b/tests/bot/cogs/test_snekbox.py @@ -21,7 +21,10 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): """Post the eval code to the URLs.snekbox_eval_api endpoint.""" resp = MagicMock() resp.json = AsyncMock(return_value="return") - self.bot.http_session.post().__aenter__.return_value = resp + + context_manager = MagicMock() + context_manager.__aenter__.return_value = resp + self.bot.http_session.post.return_value = context_manager self.assertEqual(await self.cog.post_eval("import random"), "return") self.bot.http_session.post.assert_called_with( @@ -41,7 +44,10 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): key = "MarkDiamond" resp = MagicMock() resp.json = AsyncMock(return_value={"key": key}) - self.bot.http_session.post().__aenter__.return_value = resp + + context_manager = MagicMock() + context_manager.__aenter__.return_value = resp + self.bot.http_session.post.return_value = context_manager self.assertEqual( await self.cog.upload_output("My awesome output"), @@ -57,7 +63,10 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): """Output upload gracefully fallback if the upload fail.""" resp = MagicMock() resp.json = AsyncMock(side_effect=Exception) - self.bot.http_session.post().__aenter__.return_value = resp + + context_manager = MagicMock() + context_manager.__aenter__.return_value = resp + self.bot.http_session.post.return_value = context_manager log = logging.getLogger("bot.cogs.snekbox") with self.assertLogs(logger=log, level='ERROR'): -- cgit v1.2.3 From 6aed2f6b69b79b5a7e5f327819d026e7a63a7dab Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Fri, 22 May 2020 16:15:23 -0700 Subject: Fix unawaited coro warning when instantiating Bot for MockBot's spec The fix is to mock the loop and pass it to the Bot. It will then set it as `self.loop` rather than trying to get an event loop from asyncio. The `create_task` patch has been moved to this loop mock rather than being done in MockBot to ensure that it applies to anything calling it when instantiating the Bot. --- tests/helpers.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 2b79a6c2a..2efeff7db 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -4,6 +4,7 @@ import collections import itertools import logging import unittest.mock +from asyncio import AbstractEventLoop from typing import Iterable, Optional import discord @@ -264,10 +265,16 @@ class MockAPIClient(CustomMockMixin, unittest.mock.MagicMock): spec_set = APIClient -# Create a Bot instance to get a realistic MagicMock of `discord.ext.commands.Bot` -bot_instance = Bot(command_prefix=unittest.mock.MagicMock()) -bot_instance.http_session = None -bot_instance.api_client = None +def _get_mock_loop() -> unittest.mock.Mock: + """Return a mocked asyncio.AbstractEventLoop.""" + loop = unittest.mock.create_autospec(spec=AbstractEventLoop, spec_set=True) + + # Since calling `create_task` on our MockBot does not actually schedule the coroutine object + # as a task in the asyncio loop, this `side_effect` calls `close()` on the coroutine object + # to prevent "has not been awaited"-warnings. + loop.create_task.side_effect = lambda coroutine: coroutine.close() + + return loop class MockBot(CustomMockMixin, unittest.mock.MagicMock): @@ -277,17 +284,14 @@ class MockBot(CustomMockMixin, unittest.mock.MagicMock): Instances of this class will follow the specifications of `discord.ext.commands.Bot` instances. For more information, see the `MockGuild` docstring. """ - spec_set = bot_instance + spec_set = Bot(command_prefix=unittest.mock.MagicMock(), loop=_get_mock_loop()) additional_spec_asyncs = ("wait_for",) def __init__(self, **kwargs) -> None: super().__init__(**kwargs) - self.api_client = MockAPIClient() - # Since calling `create_task` on our MockBot does not actually schedule the coroutine object - # as a task in the asyncio loop, this `side_effect` calls `close()` on the coroutine object - # to prevent "has not been awaited"-warnings. - self.loop.create_task.side_effect = lambda coroutine: coroutine.close() + self.loop = _get_mock_loop() + self.api_client = MockAPIClient(loop=self.loop) # Create a TextChannel instance to get a realistic MagicMock of `discord.TextChannel` -- cgit v1.2.3 From 1ad7833d800918efca06e5d6b2fbafdb0d757009 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Fri, 22 May 2020 16:23:12 -0700 Subject: Properly mock the redis pool in MockBot Because some of the redis pool/connection methods return futures rather than being coroutines, the redis pool had to be mocked using the CustomMockMixin so it could take advantage of `additional_spec_asyncs` to use AsyncMocks for these future-returning methods. --- tests/bot/utils/test_redis_cache.py | 12 +++--------- tests/helpers.py | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 9 deletions(-) (limited to 'tests') diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index f6344803f..991225481 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -1,11 +1,9 @@ -import asyncio import unittest -from unittest.mock import MagicMock import fakeredis.aioredis -from bot.bot import Bot from bot.utils import RedisCache +from tests import helpers class RedisCacheTests(unittest.IsolatedAsyncioTestCase): @@ -15,12 +13,8 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): async def asyncSetUp(self): # noqa: N802 - this special method can't be all lowercase """Sets up the objects that only have to be initialized once.""" - self.bot = MagicMock( - spec=Bot, - redis_session=await fakeredis.aioredis.create_redis_pool(), - _redis_ready=asyncio.Event(), - ) - self.bot._redis_ready.set() + self.bot = helpers.MockBot() + self.bot.redis_session = await fakeredis.aioredis.create_redis_pool() def test_class_attribute_namespace(self): """Test that RedisDict creates a namespace automatically for class attributes.""" diff --git a/tests/helpers.py b/tests/helpers.py index 2efeff7db..33d4f787c 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -7,6 +7,7 @@ import unittest.mock from asyncio import AbstractEventLoop from typing import Iterable, Optional +import aioredis.abc import discord from discord.ext.commands import Context @@ -265,6 +266,17 @@ class MockAPIClient(CustomMockMixin, unittest.mock.MagicMock): spec_set = APIClient +class MockRedisPool(CustomMockMixin, unittest.mock.MagicMock): + """ + A MagicMock subclass to mock an aioredis connection pool. + + Instances of this class will follow the specifications of `aioredis.abc.AbcPool` instances. + For more information, see the `MockGuild` docstring. + """ + spec_set = aioredis.abc.AbcPool + additional_spec_asyncs = ("execute", "execute_pubsub") + + def _get_mock_loop() -> unittest.mock.Mock: """Return a mocked asyncio.AbstractEventLoop.""" loop = unittest.mock.create_autospec(spec=AbstractEventLoop, spec_set=True) @@ -293,6 +305,10 @@ class MockBot(CustomMockMixin, unittest.mock.MagicMock): self.loop = _get_mock_loop() self.api_client = MockAPIClient(loop=self.loop) + # fakeredis can't be used cause it'd require awaiting a coroutine to create the pool, + # which cannot be done here in __init__. + self.redis_session = MockRedisPool() + # Create a TextChannel instance to get a realistic MagicMock of `discord.TextChannel` channel_data = { -- cgit v1.2.3 From d8f1634ab68b2cd480d57c8b9da8834866b5c9cc Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Fri, 22 May 2020 16:25:10 -0700 Subject: Use autospecced mocks in MockBot for the stats and aiohttp session This will help catch anything that tries to get/set an attribute/method which doesn't exist. It'll also catch missing/too many parameters being passed to methods. --- tests/helpers.py | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 33d4f787c..d226be3f0 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -9,9 +9,11 @@ from typing import Iterable, Optional import aioredis.abc import discord +from aiohttp import ClientSession from discord.ext.commands import Context from bot.api import APIClient +from bot.async_stats import AsyncStatsClient from bot.bot import Bot @@ -304,6 +306,8 @@ class MockBot(CustomMockMixin, unittest.mock.MagicMock): self.loop = _get_mock_loop() self.api_client = MockAPIClient(loop=self.loop) + self.http_session = unittest.mock.create_autospec(spec=ClientSession, spec_set=True) + self.stats = unittest.mock.create_autospec(spec=AsyncStatsClient, spec_set=True) # fakeredis can't be used cause it'd require awaiting a coroutine to create the pool, # which cannot be done here in __init__. -- cgit v1.2.3 From eb63fb02a49bf1979afd04a1350304edf00d3a56 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 23 May 2020 02:06:27 +0200 Subject: Finish .set and .get, and add tests. The .set and .get will accept ints, floats, and strings. These will be converted into "typestrings", which is basically just a simple format that's been invented for this object. For example, an int looks like `b"i|2423"`. Note how it is still stored as a bytestring (like everything in Redis), but because of this prefix we are able to coerce it into the type we want on the way out of the db. --- bot/utils/redis_cache.py | 72 ++++++++++++++++++++++++++++++------- tests/bot/utils/test_redis_cache.py | 36 +++++++++++++------ tests/helpers.py | 2 +- 3 files changed, 85 insertions(+), 25 deletions(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 483bbc2cd..24f2f2e03 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -1,12 +1,10 @@ from __future__ import annotations -from enum import Enum -from typing import Any, AsyncIterator, Dict, List, Optional, Tuple, Union +from typing import Any, AsyncIterator, Dict, Optional, Union from bot.bot import Bot -ValidRedisKey = Union[str, int, float] -JSONSerializableType = Optional[Union[str, float, bool, Dict, List, Tuple, Enum]] +ValidRedisType = Union[str, int, float] class RedisCache: @@ -41,7 +39,39 @@ class RedisCache: self._namespaces.append(namespace) self._namespace = namespace - def __set_name__(self, owner: object, attribute_name: str) -> None: + @staticmethod + def _to_typestring(value: ValidRedisType) -> str: + """Turn a valid Redis type into a typestring.""" + if isinstance(value, float): + return f"f|{value}" + elif isinstance(value, int): + return f"i|{value}" + elif isinstance(value, str): + return f"s|{value}" + + @staticmethod + def _from_typestring(value: str) -> ValidRedisType: + """Turn a valid Redis type into a typestring.""" + if value.startswith("f|"): + return float(value[2:]) + if value.startswith("i|"): + return int(value[2:]) + if value.startswith("s|"): + return value[2:] + + async def _validate_cache(self) -> None: + """Validate that the RedisCache is ready to be used.""" + if self.bot is None: + raise RuntimeError("Critical error: RedisCache has no `Bot` instance.") + + if self._namespace is None: + raise RuntimeError( + "Critical error: RedisCache has no namespace. " + "Did you initialize this object as a class attribute?" + ) + await self.bot._redis_ready.wait() + + def __set_name__(self, owner: Any, attribute_name: str) -> None: """ Set the namespace to Class.attribute_name. @@ -54,8 +84,11 @@ class RedisCache: if self.bot: return self + if self._namespace is None: + raise RuntimeError("RedisCache must be a class attribute.") + if instance is None: - raise NotImplementedError("You must create an instance of RedisCache to use it.") + raise RuntimeError("You must create an instance of RedisCache to use it.") for attribute in vars(instance).values(): if isinstance(attribute, Bot): @@ -69,19 +102,32 @@ class RedisCache: """Return a beautiful representation of this object instance.""" return f"RedisCache(namespace={self._namespace!r})" - async def set(self, key: ValidRedisKey, value: JSONSerializableType) -> None: + async def set(self, key: ValidRedisType, value: ValidRedisType) -> None: """Store an item in the Redis cache.""" - # await self._redis.hset(self._namespace, key, value) + await self._validate_cache() + + # Convert to a typestring and then set it + key = self._to_typestring(key) + value = self._to_typestring(value) + await self._redis.hset(self._namespace, key, value) - async def get(self, key: ValidRedisKey, default: Optional[JSONSerializableType] = None) -> JSONSerializableType: + async def get(self, key: ValidRedisType, default: Optional[ValidRedisType] = None) -> ValidRedisType: """Get an item from the Redis cache.""" - # value = await self._redis.hget(self._namespace, key) + await self._validate_cache() + key = self._to_typestring(key) + value = await self._redis.hget(self._namespace, key) + + if value is None: + return default + else: + value = self._from_typestring(value.decode("utf-8")) + return value - async def delete(self, key: ValidRedisKey) -> None: + async def delete(self, key: ValidRedisType) -> None: """Delete an item from the Redis cache.""" # await self._redis.hdel(self._namespace, key) - async def contains(self, key: ValidRedisKey) -> bool: + async def contains(self, key: ValidRedisType) -> bool: """Check if a key exists in the Redis cache.""" # return await self._redis.hexists(self._namespace, key) @@ -103,7 +149,7 @@ class RedisCache: """Deletes the entire hash from the Redis cache.""" # await self._redis.delete(self._namespace) - async def pop(self, key: ValidRedisKey, default: Optional[JSONSerializableType] = None) -> JSONSerializableType: + async def pop(self, key: ValidRedisType, default: Optional[ValidRedisType] = None) -> ValidRedisType: """Get the item, remove it from the cache, and provide a default if not found.""" # value = await self.get(key, default) # await self.delete(key) diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 991225481..ad38bfde0 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -7,27 +7,41 @@ from tests import helpers class RedisCacheTests(unittest.IsolatedAsyncioTestCase): - """Tests the RedisDict class from utils.redis_dict.py.""" + """Tests the RedisCache class from utils.redis_dict.py.""" redis = RedisCache() - async def asyncSetUp(self): # noqa: N802 - this special method can't be all lowercase + async def asyncSetUp(self): # noqa: N802 """Sets up the objects that only have to be initialized once.""" self.bot = helpers.MockBot() self.bot.redis_session = await fakeredis.aioredis.create_redis_pool() - def test_class_attribute_namespace(self): + async def test_class_attribute_namespace(self): """Test that RedisDict creates a namespace automatically for class attributes.""" self.assertEqual(self.redis._namespace, "RedisCacheTests.redis") - # Test that errors are raised when this isn't true. - # def test_set_get_item(self): - # """Test that users can set and get items from the RedisDict.""" - # self.redis['favorite_fruit'] = 'melon' - # self.redis['favorite_number'] = 86 - # self.assertEqual(self.redis['favorite_fruit'], 'melon') - # self.assertEqual(self.redis['favorite_number'], 86) - # + # Test that errors are raised when not assigned as a class attribute + bad_cache = RedisCache() + + with self.assertRaises(RuntimeError): + await bad_cache.set("test", "me_up_deadman") + + async def test_set_get_item(self): + """Test that users can set and get items from the RedisDict.""" + test_cases = ( + ('favorite_fruit', 'melon'), + ('favorite_number', 86), + ('favorite_fraction', 86.54) + ) + + # Test that we can get and set different types. + for test in test_cases: + await self.redis.set(*test) + self.assertEqual(await self.redis.get(test[0]), test[1]) + + # Test that .get allows a default value + self.assertEqual(await self.redis.get('favorite_nothing', "bearclaw"), "bearclaw") + # def test_set_item_types(self): # """Test that setitem rejects keys and values that are not strings, ints or floats.""" # fruits = ["lemon", "melon", "apple"] diff --git a/tests/helpers.py b/tests/helpers.py index d226be3f0..2b176db79 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -299,7 +299,7 @@ class MockBot(CustomMockMixin, unittest.mock.MagicMock): For more information, see the `MockGuild` docstring. """ spec_set = Bot(command_prefix=unittest.mock.MagicMock(), loop=_get_mock_loop()) - additional_spec_asyncs = ("wait_for",) + additional_spec_asyncs = ("wait_for", "_redis_ready") def __init__(self, **kwargs) -> None: super().__init__(**kwargs) -- cgit v1.2.3 From 387bf5c6b6a21e25c4fc690fb992b6b3e4c165a6 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 23 May 2020 11:36:12 +0200 Subject: Complete asyncified test suite for RedisCache This commit just alters existing code to work with the new interface, and with async. All tests are passing successfully. --- tests/bot/utils/test_redis_cache.py | 206 ++++++++++++++++++++---------------- 1 file changed, 112 insertions(+), 94 deletions(-) (limited to 'tests') diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index ad38bfde0..d257e91d9 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -16,16 +16,24 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): self.bot = helpers.MockBot() self.bot.redis_session = await fakeredis.aioredis.create_redis_pool() - async def test_class_attribute_namespace(self): + def test_class_attribute_namespace(self): """Test that RedisDict creates a namespace automatically for class attributes.""" self.assertEqual(self.redis._namespace, "RedisCacheTests.redis") - # Test that errors are raised when not assigned as a class attribute + async def test_class_attribute_required(self): + """Test that errors are raised when not assigned as a class attribute.""" bad_cache = RedisCache() + self.assertIs(bad_cache._namespace, None) with self.assertRaises(RuntimeError): await bad_cache.set("test", "me_up_deadman") + def test_namespace_collision(self): + """Test that we prevent colliding namespaces.""" + bad_cache = RedisCache() + bad_cache._set_namespace("RedisCacheTests.redis") + self.assertEqual(bad_cache._namespace, "RedisCacheTests.redis_") + async def test_set_get_item(self): """Test that users can set and get items from the RedisDict.""" test_cases = ( @@ -42,95 +50,105 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): # Test that .get allows a default value self.assertEqual(await self.redis.get('favorite_nothing', "bearclaw"), "bearclaw") - # def test_set_item_types(self): - # """Test that setitem rejects keys and values that are not strings, ints or floats.""" - # fruits = ["lemon", "melon", "apple"] - # - # with self.assertRaises(DataError): - # self.redis[fruits] = "nice" - # - # def test_contains(self): - # """Test that we can reliably use the `in` operator with our RedisDict.""" - # self.redis['favorite_country'] = "Burkina Faso" - # - # self.assertIn('favorite_country', self.redis) - # self.assertNotIn('favorite_dentist', self.redis) - # - # def test_items(self): - # """Test that the RedisDict can be iterated.""" - # self.redis.clear() - # test_cases = ( - # ('favorite_turtle', 'Donatello'), - # ('second_favorite_turtle', 'Leonardo'), - # ('third_favorite_turtle', 'Raphael'), - # ) - # for key, value in test_cases: - # self.redis[key] = value - # - # # Test regular iteration - # for test_case, key in zip(test_cases, self.redis): - # value = test_case[1] - # self.assertEqual(self.redis[key], value) - # - # # Test .items iteration - # for key, value in self.redis.items(): - # self.assertEqual(self.redis[key], value) - # - # # Test .keys iteration - # for test_case, key in zip(test_cases, self.redis.keys()): - # value = test_case[1] - # self.assertEqual(self.redis[key], value) - # - # def test_length(self): - # """Test that we can get the correct len() from the RedisDict.""" - # self.redis.clear() - # self.redis['one'] = 1 - # self.redis['two'] = 2 - # self.redis['three'] = 3 - # self.assertEqual(len(self.redis), 3) - # - # self.redis['four'] = 4 - # self.assertEqual(len(self.redis), 4) - # - # def test_to_dict(self): - # """Test that the .copy method returns a workable dictionary copy.""" - # copy = self.redis.copy() - # local_copy = dict(self.redis.items()) - # self.assertIs(type(copy), dict) - # self.assertEqual(copy, local_copy) - # - # def test_clear(self): - # """Test that the .clear method removes the entire hash.""" - # self.redis.clear() - # self.redis['teddy'] = "with me" - # self.redis['in my dreams'] = "you have a weird hat" - # self.assertEqual(len(self.redis), 2) - # - # self.redis.clear() - # self.assertEqual(len(self.redis), 0) - # - # def test_pop(self): - # """Test that we can .pop an item from the RedisDict.""" - # self.redis.clear() - # self.redis['john'] = 'was afraid' - # - # self.assertEqual(self.redis.pop('john'), 'was afraid') - # self.assertEqual(self.redis.pop('pete', 'breakneck'), 'breakneck') - # self.assertEqual(len(self.redis), 0) - # - # def test_update(self): - # """Test that we can .update the RedisDict with multiple items.""" - # self.redis.clear() - # self.redis["reckfried"] = "lona" - # self.redis["bel air"] = "prince" - # self.redis.update({ - # "reckfried": "jona", - # "mega": "hungry, though", - # }) - # - # result = { - # "reckfried": "jona", - # "bel air": "prince", - # "mega": "hungry, though", - # } - # self.assertEqual(self.redis.copy(), result) + async def test_set_item_type(self): + """Test that .set rejects keys and values that are not strings, ints or floats.""" + fruits = ["lemon", "melon", "apple"] + + with self.assertRaises(TypeError): + await self.redis.set(fruits, "nice") + + async def test_delete_item(self): + """Test that .delete allows us to delete stuff from the RedisCache.""" + # Add an item and verify that it gets added + await self.redis.set("internet", "firetruck") + self.assertEqual(await self.redis.get("internet"), "firetruck") + + # Delete that item and verify that it gets deleted + await self.redis.delete("internet") + self.assertIs(await self.redis.get("internet"), None) + + async def test_contains(self): + """Test that we can check membership with .contains.""" + await self.redis.set('favorite_country', "Burkina Faso") + + self.assertIs(await self.redis.contains('favorite_country'), True) + self.assertIs(await self.redis.contains('favorite_dentist'), False) + + async def test_items(self): + """Test that the RedisDict can be iterated.""" + await self.redis.clear() + + # Set up our test cases in the Redis cache + test_cases = [ + ('favorite_turtle', 'Donatello'), + ('second_favorite_turtle', 'Leonardo'), + ('third_favorite_turtle', 'Raphael'), + ] + for key, value in test_cases: + await self.redis.set(key, value) + + # Consume the AsyncIterator into a regular list, easier to compare that way. + redis_items = [item async for item in self.redis.items()] + + # These sequences are probably in the same order now, but probably + # isn't good enough for tests. Let's not rely on .hgetall always + # returning things in sequence, and just sort both lists to be safe. + redis_items = sorted(redis_items) + test_cases = sorted(test_cases) + + # If these are equal now, everything works fine. + self.assertSequenceEqual(test_cases, redis_items) + + async def test_length(self): + """Test that we can get the correct .length from the RedisDict.""" + await self.redis.clear() + await self.redis.set('one', 1) + await self.redis.set('two', 2) + await self.redis.set('three', 3) + self.assertEqual(await self.redis.length(), 3) + + await self.redis.set('four', 4) + self.assertEqual(await self.redis.length(), 4) + + async def test_to_dict(self): + """Test that the .copy method returns a workable dictionary copy.""" + copy = await self.redis.to_dict() + local_copy = {key: value async for key, value in self.redis.items()} + self.assertIs(type(copy), dict) + self.assertDictEqual(copy, local_copy) + + async def test_clear(self): + """Test that the .clear method removes the entire hash.""" + await self.redis.clear() + await self.redis.set('teddy', 'with me') + await self.redis.set('in my dreams', 'you have a weird hat') + self.assertEqual(await self.redis.length(), 2) + + await self.redis.clear() + self.assertEqual(await self.redis.length(), 0) + + async def test_pop(self): + """Test that we can .pop an item from the RedisDict.""" + await self.redis.clear() + await self.redis.set('john', 'was afraid') + + self.assertEqual(await self.redis.pop('john'), 'was afraid') + self.assertEqual(await self.redis.pop('pete', 'breakneck'), 'breakneck') + self.assertEqual(await self.redis.length(), 0) + + async def test_update(self): + """Test that we can .update the RedisDict with multiple items.""" + await self.redis.clear() + await self.redis.set("reckfried", "lona") + await self.redis.set("bel air", "prince") + await self.redis.update({ + "reckfried": "jona", + "mega": "hungry, though", + }) + + result = { + "reckfried": "jona", + "bel air": "prince", + "mega": "hungry, though", + } + self.assertDictEqual(await self.redis.to_dict(), result) -- cgit v1.2.3 From aa0bb028ed889d93376981213673053a540e137c Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 23 May 2020 14:30:56 +0200 Subject: Fix typo in test_to_dict docstring --- tests/bot/utils/test_redis_cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index d257e91d9..2ce57499a 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -111,7 +111,7 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(await self.redis.length(), 4) async def test_to_dict(self): - """Test that the .copy method returns a workable dictionary copy.""" + """Test that the .to_dict method returns a workable dictionary copy.""" copy = await self.redis.to_dict() local_copy = {key: value async for key, value in self.redis.items()} self.assertIs(type(copy), dict) -- cgit v1.2.3 From 5120717a47c07812d1631cf0905ff3062e139487 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 23 May 2020 15:17:57 +0200 Subject: DRY approach to typestring prefix resolution Thanks to @kwzrd for this idea, basically we're making a constant with the typestring prefixes and iterating that in all our converters. These converter functions will also now raise TypeErrors if we try to convert something that isn't in this constants list. I've also added a new test that tests this functionality. --- bot/utils/redis_cache.py | 54 ++++++++++++++++++++++++++----------- tests/bot/utils/test_redis_cache.py | 21 +++++++++++++++ 2 files changed, 60 insertions(+), 15 deletions(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 6831be157..1ec1b9fea 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -4,6 +4,11 @@ from typing import Any, AsyncIterator, Dict, Optional, Union from bot.bot import Bot +TYPESTRING_PREFIXES = ( + ("f|", float), + ("i|", int), + ("s|", str), +) ValidRedisType = Union[str, int, float] @@ -78,26 +83,45 @@ class RedisCache: self._namespace = namespace @staticmethod - def _to_typestring(value: ValidRedisType) -> str: - """Turn a valid Redis type into a typestring.""" - if isinstance(value, float): - return f"f|{value}" - elif isinstance(value, int): - return f"i|{value}" - elif isinstance(value, str): - return f"s|{value}" + def _valid_typestring_types() -> str: + """ + Creates a nice, readable list of valid types for typestrings, useful for error messages. + + This will be dynamically updated if we change the TYPESTRING_PREFIXES constant up top. + """ + valid_types = ", ".join([str(_type).split("'")[1] for _, _type in TYPESTRING_PREFIXES]) + valid_types = ", and ".join(valid_types.rsplit(", ", 1)) + return valid_types @staticmethod - def _from_typestring(value: Union[bytes, str]) -> ValidRedisType: + def _valid_typestring_prefixes() -> str: + """ + Creates a nice, readable list of valid prefixes for typestrings, useful for error messages. + + This will be dynamically updated if we change the TYPESTRING_PREFIXES constant up top. + """ + valid_prefixes = ", ".join([f"'{prefix}'" for prefix, _ in TYPESTRING_PREFIXES]) + valid_prefixes = ", and ".join(valid_prefixes.rsplit(", ", 1)) + return valid_prefixes + + def _to_typestring(self, value: ValidRedisType) -> str: + """Turn a valid Redis type into a typestring.""" + for prefix, _type in TYPESTRING_PREFIXES: + if isinstance(value, _type): + return f"{prefix}{value}" + raise TypeError(f"RedisCache._from_typestring only supports the types {self._valid_typestring_types()}.") + + def _from_typestring(self, value: Union[bytes, str]) -> ValidRedisType: """Turn a typestring into a valid Redis type.""" + # Stuff that comes out of Redis will be bytestrings, so let's decode those. if isinstance(value, bytes): value = value.decode('utf-8') - if value.startswith("f|"): - return float(value[2:]) - if value.startswith("i|"): - return int(value[2:]) - if value.startswith("s|"): - return value[2:] + + # Now we convert our unicode string back into the type it originally was. + for prefix, _type in TYPESTRING_PREFIXES: + if value.startswith(prefix): + return _type(value[2:]) + raise TypeError(f"RedisCache._to_typestring only supports the prefixes {self._valid_typestring_prefixes()}.") def _dict_from_typestring(self, dictionary: Dict) -> Dict: """Turns all contents of a dict into valid Redis types.""" diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 2ce57499a..150195726 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -152,3 +152,24 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): "mega": "hungry, though", } self.assertDictEqual(await self.redis.to_dict(), result) + + def test_typestring_conversion(self): + """Test the typestring-related helper functions.""" + conversion_tests = ( + (12, "i|12"), + (12.4, "f|12.4"), + ("cowabunga", "s|cowabunga"), + ) + + # Test conversion to typestring + for _input, expected in conversion_tests: + self.assertEqual(self.redis._to_typestring(_input), expected) + + # Test conversion from typestrings + for _input, expected in conversion_tests: + self.assertEqual(self.redis._from_typestring(expected), _input) + + # Test that exceptions are raised on invalid input + with self.assertRaises(TypeError): + self.redis._to_typestring(["internet"]) + self.redis._from_typestring("o|firedog") -- cgit v1.2.3 From a52a13020f3468c671cb549052a9c8e303ae9d8c Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 23 May 2020 10:27:32 -0700 Subject: Remove redis session mock from MockBot It's not feasible to mock it because all the commands return futures rather than being coroutines, so they cannot automatically be turned into AsyncMocks. Furthermore, no code should ever use the redis session directly besides RedisCache. Since the tests for RedisCache already use fakeredis, there's no use in trying to mock redis in MockBot. --- tests/helpers.py | 16 ---------------- 1 file changed, 16 deletions(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 2b176db79..5ad826156 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -7,7 +7,6 @@ import unittest.mock from asyncio import AbstractEventLoop from typing import Iterable, Optional -import aioredis.abc import discord from aiohttp import ClientSession from discord.ext.commands import Context @@ -268,17 +267,6 @@ class MockAPIClient(CustomMockMixin, unittest.mock.MagicMock): spec_set = APIClient -class MockRedisPool(CustomMockMixin, unittest.mock.MagicMock): - """ - A MagicMock subclass to mock an aioredis connection pool. - - Instances of this class will follow the specifications of `aioredis.abc.AbcPool` instances. - For more information, see the `MockGuild` docstring. - """ - spec_set = aioredis.abc.AbcPool - additional_spec_asyncs = ("execute", "execute_pubsub") - - def _get_mock_loop() -> unittest.mock.Mock: """Return a mocked asyncio.AbstractEventLoop.""" loop = unittest.mock.create_autospec(spec=AbstractEventLoop, spec_set=True) @@ -309,10 +297,6 @@ class MockBot(CustomMockMixin, unittest.mock.MagicMock): self.http_session = unittest.mock.create_autospec(spec=ClientSession, spec_set=True) self.stats = unittest.mock.create_autospec(spec=AsyncStatsClient, spec_set=True) - # fakeredis can't be used cause it'd require awaiting a coroutine to create the pool, - # which cannot be done here in __init__. - self.redis_session = MockRedisPool() - # Create a TextChannel instance to get a realistic MagicMock of `discord.TextChannel` channel_data = { -- cgit v1.2.3 From b2009d5304beba4829b7727ca154bb6a0d1cd50a Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 24 May 2020 11:42:33 +0200 Subject: Make .items return ItemsView instead of AsyncIter There really was no compelling reason why this method should return an AsyncIterator or than that `async for items in cache.items()` has nice readability, but there were a few concerns. One is a concern about race conditions raised by @SebastiaanZ, and @MarkKoz raised a concern that it was misleading to have an AsyncIterator that only "pretended" to be lazy. To address these concerns, I've refactored it to return a regular ItemsView instead. I also improved the docstring, and fixed the relevant tests. --- bot/utils/redis_cache.py | 28 +++++++++++++++++++++------- tests/bot/utils/test_redis_cache.py | 4 ++-- 2 files changed, 23 insertions(+), 9 deletions(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 558ab33a7..fb9a534bd 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Any, AsyncIterator, Dict, Optional, Union +from typing import Any, Dict, ItemsView, Optional, Union from bot.bot import Bot @@ -237,12 +237,26 @@ class RedisCache: key = self._to_typestring(key) return await self._redis.hexists(self._namespace, key) - async def items(self) -> AsyncIterator: - """Iterate all the items in the Redis cache.""" + async def items(self) -> ItemsView: + """ + Fetch all the key/value pairs in the cache. + + Returns a normal ItemsView, like you would get from dict.items(). + + Keep in mind that these items are just a _copy_ of the data in the + RedisCache - any changes you make to them will not be reflected + into the RedisCache itself. If you want to change these, you need + to make a .set call. + + Example: + items = await my_cache.items() + for key, value in items: + # Iterate like a normal dictionary + """ await self._validate_cache() - data = await self._redis.hgetall(self._namespace) # Get all the keys - for key, value in self._dict_from_typestring(data).items(): - yield key, value + return self._dict_from_typestring( + await self._redis.hgetall(self._namespace) + ).items() async def length(self) -> int: """Return the number of items in the Redis cache.""" @@ -251,7 +265,7 @@ class RedisCache: async def to_dict(self) -> Dict: """Convert to dict and return.""" - return {key: value async for key, value in self.items()} + return {key: value for key, value in await self.items()} async def clear(self) -> None: """Deletes the entire hash from the Redis cache.""" diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 150195726..6e12002ed 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -88,7 +88,7 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): await self.redis.set(key, value) # Consume the AsyncIterator into a regular list, easier to compare that way. - redis_items = [item async for item in self.redis.items()] + redis_items = [item for item in await self.redis.items()] # These sequences are probably in the same order now, but probably # isn't good enough for tests. Let's not rely on .hgetall always @@ -113,7 +113,7 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): async def test_to_dict(self): """Test that the .to_dict method returns a workable dictionary copy.""" copy = await self.redis.to_dict() - local_copy = {key: value async for key, value in self.redis.items()} + local_copy = {key: value for key, value in await self.redis.items()} self.assertIs(type(copy), dict) self.assertDictEqual(copy, local_copy) -- cgit v1.2.3 From 01bedcadf762262eef0a2b406faf66cdc16a5c85 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 24 May 2020 13:04:41 +0200 Subject: Add .increment and .decrement methods. Sometimes, we just want to store a counter in the cache. In this case, it is convenient to have a single method that will allow us to increment or decrement this counter. These methods allow you to decrement or increment floats and integers by an specified amount. By default, it'll increment or decrement by 1. Since this involves several API requests, we create an asyncio.Lock so that we don't end up with race conditions. --- bot/utils/redis_cache.py | 35 +++++++++++++++++++++++++++++++++++ tests/bot/utils/test_redis_cache.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index fb9a534bd..290fae1a0 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -1,5 +1,6 @@ from __future__ import annotations +import asyncio from typing import Any, Dict, ItemsView, Optional, Union from bot.bot import Bot @@ -77,6 +78,7 @@ class RedisCache: """Initialize the RedisCache.""" self._namespace = None self.bot = None + self.increment_lock = asyncio.Lock() def _set_namespace(self, namespace: str) -> None: """Try to set the namespace, but do not permit collisions.""" @@ -287,3 +289,36 @@ class RedisCache: """Update the Redis cache with multiple values.""" await self._validate_cache() await self._redis.hmset_dict(self._namespace, self._dict_to_typestring(items)) + + async def increment(self, key: RedisType, amount: Optional[int, float] = 1) -> None: + """ + Increment the value by `amount`. + + This works for both floats and ints, but will raise a TypeError + if you try to do it for any other type of value. + + This also supports negative amounts, although it would provide better + readability to use .decrement() for that. + """ + # Since this has several API calls, we need a lock to prevent race conditions + async with self.increment_lock: + value = await self.get(key) + + # Can't increment a non-existing value + if value is None: + raise RuntimeError("The provided key does not exist!") + + # If it does exist, and it's an int or a float, increment and set it. + if isinstance(value, int) or isinstance(value, float): + value += amount + await self.set(key, value) + else: + raise TypeError("You may only increment or decrement values that are integers or floats.") + + async def decrement(self, key: RedisType, amount: Optional[int, float] = 1) -> None: + """ + Decrement the value by `amount`. + + Basically just does the opposite of .increment. + """ + await self.increment(key, -amount) diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 6e12002ed..dbbaef018 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -173,3 +173,37 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): with self.assertRaises(TypeError): self.redis._to_typestring(["internet"]) self.redis._from_typestring("o|firedog") + + async def test_increment_decrement(self): + """Test .increment and .decrement methods.""" + await self.redis.set("entropic", 5) + await self.redis.set("disentropic", 12.5) + + # Test default increment + await self.redis.increment("entropic") + self.assertEqual(await self.redis.get("entropic"), 6) + + # Test default decrement + await self.redis.decrement("entropic") + self.assertEqual(await self.redis.get("entropic"), 5) + + # Test float increment with float + await self.redis.increment("disentropic", 2.0) + self.assertEqual(await self.redis.get("disentropic"), 14.5) + + # Test float increment with int + await self.redis.increment("disentropic", 2) + self.assertEqual(await self.redis.get("disentropic"), 16.5) + + # Test negative increments, because why not. + await self.redis.increment("entropic", -5) + self.assertEqual(await self.redis.get("entropic"), 0) + + # Negative decrements? Sure. + await self.redis.decrement("entropic", -5) + self.assertEqual(await self.redis.get("entropic"), 5) + + # What about if we use a negative float to decrement an int? + # This should convert the type into a float. + await self.redis.decrement("entropic", -2.5) + self.assertEqual(await self.redis.get("entropic"), 7.5) -- cgit v1.2.3 From c5e6e8f796265ee6faebdd3d02c839972cd028a9 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 24 May 2020 13:49:20 +0200 Subject: MockBot needs to be aware of redis_ready Forgot to update the additional_spec_asyncs when changing the name of this Bot attribute to be public. --- tests/helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 5ad826156..13283339b 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -287,7 +287,7 @@ class MockBot(CustomMockMixin, unittest.mock.MagicMock): For more information, see the `MockGuild` docstring. """ spec_set = Bot(command_prefix=unittest.mock.MagicMock(), loop=_get_mock_loop()) - additional_spec_asyncs = ("wait_for", "_redis_ready") + additional_spec_asyncs = ("wait_for", "redis_ready") def __init__(self, **kwargs) -> None: super().__init__(**kwargs) -- cgit v1.2.3 From ad8b1fa455e141074daec5047682e82ed96db1f5 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 24 May 2020 19:09:45 +0200 Subject: Improve error and error testing for increment Changed a RuntimeError to a KeyError (thanks @MarkKoz), and also added some tests to ensure that the right errors are raised whenever this method is used incorrectly. --- bot/utils/redis_cache.py | 2 +- tests/bot/utils/test_redis_cache.py | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 5fc34d464..b91d663f3 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -341,7 +341,7 @@ class RedisCache: # Can't increment a non-existing value if value is None: log.exception("Attempt to increment/decrement value for non-existent key.") - raise RuntimeError("The provided key does not exist!") + raise KeyError("The provided key does not exist!") # If it does exist, and it's an int or a float, increment and set it. if isinstance(value, int) or isinstance(value, float): diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index dbbaef018..7405487ed 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -207,3 +207,11 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): # This should convert the type into a float. await self.redis.decrement("entropic", -2.5) self.assertEqual(await self.redis.get("entropic"), 7.5) + + # Let's test that they raise the right errors + with self.assertRaises(KeyError): + await self.redis.increment("doesn't_exist!") + + await self.redis.set("stringthing", "stringthing") + with self.assertRaises(TypeError): + await self.redis.increment("stringthing") -- cgit v1.2.3 From 856cecbd2354d4cbdbace5a39b7eb9e3d3bf23c7 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 24 May 2020 19:29:13 -0700 Subject: Add support for Union type annotations for constants Note that `Optional[x]` is just an alias for `Union[None, x]` so this effectively supports `Optional` too. This was especially troublesome because the redis password must be unset/None in order to avoid authentication, but the test would complain that `None` isn't a `str`. Setting to an empty string would pass the test but then make redis authenticate and fail. --- bot/constants.py | 14 +++++++------- tests/bot/test_constants.py | 17 ++++++++++++----- 2 files changed, 19 insertions(+), 12 deletions(-) (limited to 'tests') diff --git a/bot/constants.py b/bot/constants.py index 75d394b6a..145ae54db 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -15,7 +15,7 @@ import os from collections.abc import Mapping from enum import Enum from pathlib import Path -from typing import Dict, List +from typing import Dict, List, Optional import yaml @@ -198,7 +198,7 @@ class Bot(metaclass=YAMLGetter): prefix: str token: str - sentry_dsn: str + sentry_dsn: Optional[str] class Redis(metaclass=YAMLGetter): @@ -207,7 +207,7 @@ class Redis(metaclass=YAMLGetter): host: str port: int - password: str + password: Optional[str] use_fakeredis: bool # If this is True, Bot will use fakeredis.aioredis @@ -459,7 +459,7 @@ class Guild(metaclass=YAMLGetter): class Keys(metaclass=YAMLGetter): section = "keys" - site_api: str + site_api: Optional[str] class URLs(metaclass=YAMLGetter): @@ -502,8 +502,8 @@ class Reddit(metaclass=YAMLGetter): section = "reddit" subreddits: list - client_id: str - secret: str + client_id: Optional[str] + secret: Optional[str] class Wolfram(metaclass=YAMLGetter): @@ -511,7 +511,7 @@ class Wolfram(metaclass=YAMLGetter): user_limit_day: int guild_limit_day: int - key: str + key: Optional[str] class AntiSpam(metaclass=YAMLGetter): diff --git a/tests/bot/test_constants.py b/tests/bot/test_constants.py index dae7c066c..db9a9bcb0 100644 --- a/tests/bot/test_constants.py +++ b/tests/bot/test_constants.py @@ -1,4 +1,5 @@ import inspect +import typing import unittest from bot import constants @@ -8,7 +9,7 @@ class ConstantsTests(unittest.TestCase): """Tests for our constants.""" def test_section_configuration_matches_type_specification(self): - """The section annotations should match the actual types of the sections.""" + """"The section annotations should match the actual types of the sections.""" sections = ( cls @@ -19,8 +20,14 @@ class ConstantsTests(unittest.TestCase): for name, annotation in section.__annotations__.items(): with self.subTest(section=section, name=name, annotation=annotation): value = getattr(section, name) + annotation_args = typing.get_args(annotation) - if getattr(annotation, '_name', None) in ('Dict', 'List'): - self.skipTest("Cannot validate containers yet.") - - self.assertIsInstance(value, annotation) + if not annotation_args: + self.assertIsInstance(value, annotation) + else: + origin = typing.get_origin(annotation) + if origin is typing.Union: + is_instance = any(isinstance(value, arg) for arg in annotation_args) + self.assertTrue(is_instance) + else: + self.skipTest(f"Validating type {annotation} is unsupported.") -- cgit v1.2.3 From 9b9aa9b2adbdcd0e0b8c4f4ad38f112a9566fa2f Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 12:03:09 -0700 Subject: Support validating collection types for constants This is a simple validation that only check the type of the collection. It does not validate the types inside the collection because that has proven to be quite complex. --- tests/bot/test_constants.py | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) (limited to 'tests') diff --git a/tests/bot/test_constants.py b/tests/bot/test_constants.py index db9a9bcb0..2937b6189 100644 --- a/tests/bot/test_constants.py +++ b/tests/bot/test_constants.py @@ -5,6 +5,31 @@ import unittest from bot import constants +def is_annotation_instance(value: typing.Any, annotation: typing.Any) -> bool: + """ + Return True if `value` is an instance of the type represented by `annotation`. + + This doesn't account for things like Unions or checking for homogenous types in collections. + """ + origin = typing.get_origin(annotation) + + # This is done in case a bare e.g. `typing.List` is used. + # In such case, for the assertion to pass, the type needs to be normalised to e.g. `list`. + # `get_origin()` does this normalisation for us. + type_ = annotation if origin is None else origin + + return isinstance(value, type_) + + +def is_any_instance(value: typing.Any, types: typing.Collection) -> bool: + """Return True if `value` is an instance of any type in `types`.""" + for type_ in types: + if is_annotation_instance(value, type_): + return True + + return False + + class ConstantsTests(unittest.TestCase): """Tests for our constants.""" @@ -20,14 +45,13 @@ class ConstantsTests(unittest.TestCase): for name, annotation in section.__annotations__.items(): with self.subTest(section=section, name=name, annotation=annotation): value = getattr(section, name) + origin = typing.get_origin(annotation) annotation_args = typing.get_args(annotation) + failure_msg = f"{value} is not an instance of {annotation}" - if not annotation_args: - self.assertIsInstance(value, annotation) + if origin is typing.Union: + is_instance = is_any_instance(value, annotation_args) + self.assertTrue(is_instance, failure_msg) else: - origin = typing.get_origin(annotation) - if origin is typing.Union: - is_instance = any(isinstance(value, arg) for arg in annotation_args) - self.assertTrue(is_instance) - else: - self.skipTest(f"Validating type {annotation} is unsupported.") + is_instance = is_annotation_instance(value, annotation) + self.assertTrue(is_instance, failure_msg) -- cgit v1.2.3 From 87d42add019e8ef1bad5d9593f6ed5a803e4d153 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 12:04:50 -0700 Subject: Improve output of section name in config validation subtests --- tests/bot/test_constants.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/test_constants.py b/tests/bot/test_constants.py index 2937b6189..f10d6fbe8 100644 --- a/tests/bot/test_constants.py +++ b/tests/bot/test_constants.py @@ -43,7 +43,7 @@ class ConstantsTests(unittest.TestCase): ) for section in sections: for name, annotation in section.__annotations__.items(): - with self.subTest(section=section, name=name, annotation=annotation): + with self.subTest(section=section.__name__, name=name, annotation=annotation): value = getattr(section, name) origin = typing.get_origin(annotation) annotation_args = typing.get_args(annotation) -- cgit v1.2.3 From 1ab34dd48fce2de70db1fb2dd6da06f752460829 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Tue, 26 May 2020 19:06:57 +0200 Subject: Add a test for RuntimeErrors. This just tests that the various RuntimeErrors are reachable - that includes the error about not having a bot instance, the one about not being a class attribute, and the one about not having instantiated the class. This test addresses a concern raised by @MarkKoz in a review. I've decided not to test that actual contents of these RuntimeErrors, because I believe that sort of testing is a bit too brittle. It shouldn't break a test just to change the content of an error string. --- tests/bot/utils/test_redis_cache.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'tests') diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 7405487ed..1b05ae350 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -215,3 +215,25 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): await self.redis.set("stringthing", "stringthing") with self.assertRaises(TypeError): await self.redis.increment("stringthing") + + async def test_exceptions_raised(self): + """Testing that the various RuntimeErrors are reachable.""" + class MyCog: + cache = RedisCache() + + def __init__(self): + self.other_cache = RedisCache() + + cog = MyCog() + + # Raises "No Bot instance" + with self.assertRaises(RuntimeError): + await cog.cache.get("john") + + # Raises "RedisCache has no namespace" + with self.assertRaises(RuntimeError): + await cog.other_cache.get("was") + + # Raises "You must access the RedisCache instance through the cog instance" + with self.assertRaises(RuntimeError): + await MyCog.cache.get("afraid") -- cgit v1.2.3 From 35a1de37307b1745c061e490be4e96c8467de212 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 27 May 2020 12:21:58 +0200 Subject: Clear cache in asyncSetUp instead of tests. --- tests/bot/utils/test_redis_cache.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) (limited to 'tests') diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 1b05ae350..900a6d035 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -15,6 +15,7 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): """Sets up the objects that only have to be initialized once.""" self.bot = helpers.MockBot() self.bot.redis_session = await fakeredis.aioredis.create_redis_pool() + await self.redis.clear() def test_class_attribute_namespace(self): """Test that RedisDict creates a namespace automatically for class attributes.""" @@ -76,8 +77,6 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): async def test_items(self): """Test that the RedisDict can be iterated.""" - await self.redis.clear() - # Set up our test cases in the Redis cache test_cases = [ ('favorite_turtle', 'Donatello'), @@ -101,7 +100,6 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): async def test_length(self): """Test that we can get the correct .length from the RedisDict.""" - await self.redis.clear() await self.redis.set('one', 1) await self.redis.set('two', 2) await self.redis.set('three', 3) @@ -119,7 +117,6 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): async def test_clear(self): """Test that the .clear method removes the entire hash.""" - await self.redis.clear() await self.redis.set('teddy', 'with me') await self.redis.set('in my dreams', 'you have a weird hat') self.assertEqual(await self.redis.length(), 2) @@ -129,7 +126,6 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): async def test_pop(self): """Test that we can .pop an item from the RedisDict.""" - await self.redis.clear() await self.redis.set('john', 'was afraid') self.assertEqual(await self.redis.pop('john'), 'was afraid') @@ -138,7 +134,6 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): async def test_update(self): """Test that we can .update the RedisDict with multiple items.""" - await self.redis.clear() await self.redis.set("reckfried", "lona") await self.redis.set("bel air", "prince") await self.redis.update({ -- cgit v1.2.3 From b18930735e05e09ba615cb54fe1dbdfd41bb0f81 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 27 May 2020 12:51:40 +0200 Subject: Refactor .increment and add lock test. The way we were doing the asyncio.Lock() stuff for increment was slightly problematic. @aeros has adviced us that it's better to just initialize the lock as None in __init__, and then initialize it inside the first coroutine that uses it instead. This ensures that the correct loop gets attached to the lock, so we don't end up getting errors like this one: RuntimeError: got Future attached to a different loop This happens because the lock and the actual calling coroutines aren't on the same loop. When creating a new test, test_increment_lock, we discovered that we needed a small refactor here and also in the test class to make this new test pass. So, now we're creating a DummyCog for every test method, and this will ensure the loop streams never cross. Cause we all know we must never cross the streams. --- bot/utils/redis_cache.py | 11 ++- tests/bot/utils/test_redis_cache.py | 163 ++++++++++++++++++++++-------------- 2 files changed, 109 insertions(+), 65 deletions(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 895a12da4..33e5d5852 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -81,7 +81,7 @@ class RedisCache: """Initialize the RedisCache.""" self._namespace = None self.bot = None - self._increment_lock = asyncio.Lock() + self._increment_lock = None def _set_namespace(self, namespace: str) -> None: """Try to set the namespace, but do not permit collisions.""" @@ -345,6 +345,15 @@ class RedisCache: """ log.trace(f"Attempting to increment/decrement the value with the key {key} by {amount}.") + # We initialize the lock here, because we need to ensure we get it + # running on the same loop as the calling coroutine. + # + # If we initialized the lock in the __init__, the loop that the coroutine this method + # would be called from might not exist yet, and so the lock would be on a different + # loop, which would raise RuntimeErrors. + if self._increment_lock is None: + self._increment_lock = asyncio.Lock() + # Since this has several API calls, we need a lock to prevent race conditions async with self._increment_lock: value = await self.get(key) diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 900a6d035..efd168dac 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -1,3 +1,4 @@ +import asyncio import unittest import fakeredis.aioredis @@ -9,17 +10,30 @@ from tests import helpers class RedisCacheTests(unittest.IsolatedAsyncioTestCase): """Tests the RedisCache class from utils.redis_dict.py.""" - redis = RedisCache() - async def asyncSetUp(self): # noqa: N802 """Sets up the objects that only have to be initialized once.""" self.bot = helpers.MockBot() self.bot.redis_session = await fakeredis.aioredis.create_redis_pool() - await self.redis.clear() + + # Okay, so this is necessary so that we can create a clean new + # class for every test method, and we want that because it will + # ensure we get a fresh loop, which is necessary for test_increment_lock + # to be able to pass. + class DummyCog: + """A dummy cog, for dummies.""" + + redis = RedisCache() + + def __init__(self, bot: helpers.MockBot): + self.bot = bot + + self.cog = DummyCog(self.bot) + + await self.cog.redis.clear() def test_class_attribute_namespace(self): """Test that RedisDict creates a namespace automatically for class attributes.""" - self.assertEqual(self.redis._namespace, "RedisCacheTests.redis") + self.assertEqual(self.cog.redis._namespace, "DummyCog.redis") async def test_class_attribute_required(self): """Test that errors are raised when not assigned as a class attribute.""" @@ -31,9 +45,13 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): def test_namespace_collision(self): """Test that we prevent colliding namespaces.""" - bad_cache = RedisCache() - bad_cache._set_namespace("RedisCacheTests.redis") - self.assertEqual(bad_cache._namespace, "RedisCacheTests.redis_") + bob_cache_1 = RedisCache() + bob_cache_1._set_namespace("BobRoss") + self.assertEqual(bob_cache_1._namespace, "BobRoss") + + bob_cache_2 = RedisCache() + bob_cache_2._set_namespace("BobRoss") + self.assertEqual(bob_cache_2._namespace, "BobRoss_") async def test_set_get_item(self): """Test that users can set and get items from the RedisDict.""" @@ -45,35 +63,35 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): # Test that we can get and set different types. for test in test_cases: - await self.redis.set(*test) - self.assertEqual(await self.redis.get(test[0]), test[1]) + await self.cog.redis.set(*test) + self.assertEqual(await self.cog.redis.get(test[0]), test[1]) # Test that .get allows a default value - self.assertEqual(await self.redis.get('favorite_nothing', "bearclaw"), "bearclaw") + self.assertEqual(await self.cog.redis.get('favorite_nothing', "bearclaw"), "bearclaw") async def test_set_item_type(self): """Test that .set rejects keys and values that are not strings, ints or floats.""" fruits = ["lemon", "melon", "apple"] with self.assertRaises(TypeError): - await self.redis.set(fruits, "nice") + await self.cog.redis.set(fruits, "nice") async def test_delete_item(self): """Test that .delete allows us to delete stuff from the RedisCache.""" # Add an item and verify that it gets added - await self.redis.set("internet", "firetruck") - self.assertEqual(await self.redis.get("internet"), "firetruck") + await self.cog.redis.set("internet", "firetruck") + self.assertEqual(await self.cog.redis.get("internet"), "firetruck") # Delete that item and verify that it gets deleted - await self.redis.delete("internet") - self.assertIs(await self.redis.get("internet"), None) + await self.cog.redis.delete("internet") + self.assertIs(await self.cog.redis.get("internet"), None) async def test_contains(self): """Test that we can check membership with .contains.""" - await self.redis.set('favorite_country', "Burkina Faso") + await self.cog.redis.set('favorite_country', "Burkina Faso") - self.assertIs(await self.redis.contains('favorite_country'), True) - self.assertIs(await self.redis.contains('favorite_dentist'), False) + self.assertIs(await self.cog.redis.contains('favorite_country'), True) + self.assertIs(await self.cog.redis.contains('favorite_dentist'), False) async def test_items(self): """Test that the RedisDict can be iterated.""" @@ -84,10 +102,10 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): ('third_favorite_turtle', 'Raphael'), ] for key, value in test_cases: - await self.redis.set(key, value) + await self.cog.redis.set(key, value) # Consume the AsyncIterator into a regular list, easier to compare that way. - redis_items = [item for item in await self.redis.items()] + redis_items = [item for item in await self.cog.redis.items()] # These sequences are probably in the same order now, but probably # isn't good enough for tests. Let's not rely on .hgetall always @@ -100,43 +118,43 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): async def test_length(self): """Test that we can get the correct .length from the RedisDict.""" - await self.redis.set('one', 1) - await self.redis.set('two', 2) - await self.redis.set('three', 3) - self.assertEqual(await self.redis.length(), 3) + await self.cog.redis.set('one', 1) + await self.cog.redis.set('two', 2) + await self.cog.redis.set('three', 3) + self.assertEqual(await self.cog.redis.length(), 3) - await self.redis.set('four', 4) - self.assertEqual(await self.redis.length(), 4) + await self.cog.redis.set('four', 4) + self.assertEqual(await self.cog.redis.length(), 4) async def test_to_dict(self): """Test that the .to_dict method returns a workable dictionary copy.""" - copy = await self.redis.to_dict() - local_copy = {key: value for key, value in await self.redis.items()} + copy = await self.cog.redis.to_dict() + local_copy = {key: value for key, value in await self.cog.redis.items()} self.assertIs(type(copy), dict) self.assertDictEqual(copy, local_copy) async def test_clear(self): """Test that the .clear method removes the entire hash.""" - await self.redis.set('teddy', 'with me') - await self.redis.set('in my dreams', 'you have a weird hat') - self.assertEqual(await self.redis.length(), 2) + await self.cog.redis.set('teddy', 'with me') + await self.cog.redis.set('in my dreams', 'you have a weird hat') + self.assertEqual(await self.cog.redis.length(), 2) - await self.redis.clear() - self.assertEqual(await self.redis.length(), 0) + await self.cog.redis.clear() + self.assertEqual(await self.cog.redis.length(), 0) async def test_pop(self): """Test that we can .pop an item from the RedisDict.""" - await self.redis.set('john', 'was afraid') + await self.cog.redis.set('john', 'was afraid') - self.assertEqual(await self.redis.pop('john'), 'was afraid') - self.assertEqual(await self.redis.pop('pete', 'breakneck'), 'breakneck') - self.assertEqual(await self.redis.length(), 0) + self.assertEqual(await self.cog.redis.pop('john'), 'was afraid') + self.assertEqual(await self.cog.redis.pop('pete', 'breakneck'), 'breakneck') + self.assertEqual(await self.cog.redis.length(), 0) async def test_update(self): """Test that we can .update the RedisDict with multiple items.""" - await self.redis.set("reckfried", "lona") - await self.redis.set("bel air", "prince") - await self.redis.update({ + await self.cog.redis.set("reckfried", "lona") + await self.cog.redis.set("bel air", "prince") + await self.cog.redis.update({ "reckfried": "jona", "mega": "hungry, though", }) @@ -146,7 +164,7 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): "bel air": "prince", "mega": "hungry, though", } - self.assertDictEqual(await self.redis.to_dict(), result) + self.assertDictEqual(await self.cog.redis.to_dict(), result) def test_typestring_conversion(self): """Test the typestring-related helper functions.""" @@ -158,58 +176,75 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): # Test conversion to typestring for _input, expected in conversion_tests: - self.assertEqual(self.redis._to_typestring(_input), expected) + self.assertEqual(self.cog.redis._to_typestring(_input), expected) # Test conversion from typestrings for _input, expected in conversion_tests: - self.assertEqual(self.redis._from_typestring(expected), _input) + self.assertEqual(self.cog.redis._from_typestring(expected), _input) # Test that exceptions are raised on invalid input with self.assertRaises(TypeError): - self.redis._to_typestring(["internet"]) - self.redis._from_typestring("o|firedog") + self.cog.redis._to_typestring(["internet"]) + self.cog.redis._from_typestring("o|firedog") async def test_increment_decrement(self): """Test .increment and .decrement methods.""" - await self.redis.set("entropic", 5) - await self.redis.set("disentropic", 12.5) + await self.cog.redis.set("entropic", 5) + await self.cog.redis.set("disentropic", 12.5) # Test default increment - await self.redis.increment("entropic") - self.assertEqual(await self.redis.get("entropic"), 6) + await self.cog.redis.increment("entropic") + self.assertEqual(await self.cog.redis.get("entropic"), 6) # Test default decrement - await self.redis.decrement("entropic") - self.assertEqual(await self.redis.get("entropic"), 5) + await self.cog.redis.decrement("entropic") + self.assertEqual(await self.cog.redis.get("entropic"), 5) # Test float increment with float - await self.redis.increment("disentropic", 2.0) - self.assertEqual(await self.redis.get("disentropic"), 14.5) + await self.cog.redis.increment("disentropic", 2.0) + self.assertEqual(await self.cog.redis.get("disentropic"), 14.5) # Test float increment with int - await self.redis.increment("disentropic", 2) - self.assertEqual(await self.redis.get("disentropic"), 16.5) + await self.cog.redis.increment("disentropic", 2) + self.assertEqual(await self.cog.redis.get("disentropic"), 16.5) # Test negative increments, because why not. - await self.redis.increment("entropic", -5) - self.assertEqual(await self.redis.get("entropic"), 0) + await self.cog.redis.increment("entropic", -5) + self.assertEqual(await self.cog.redis.get("entropic"), 0) # Negative decrements? Sure. - await self.redis.decrement("entropic", -5) - self.assertEqual(await self.redis.get("entropic"), 5) + await self.cog.redis.decrement("entropic", -5) + self.assertEqual(await self.cog.redis.get("entropic"), 5) # What about if we use a negative float to decrement an int? # This should convert the type into a float. - await self.redis.decrement("entropic", -2.5) - self.assertEqual(await self.redis.get("entropic"), 7.5) + await self.cog.redis.decrement("entropic", -2.5) + self.assertEqual(await self.cog.redis.get("entropic"), 7.5) # Let's test that they raise the right errors with self.assertRaises(KeyError): - await self.redis.increment("doesn't_exist!") + await self.cog.redis.increment("doesn't_exist!") - await self.redis.set("stringthing", "stringthing") + await self.cog.redis.set("stringthing", "stringthing") with self.assertRaises(TypeError): - await self.redis.increment("stringthing") + await self.cog.redis.increment("stringthing") + + async def test_increment_lock(self): + """Test that we can't produce a race condition in .increment.""" + await self.cog.redis.set("test_key", 0) + tasks = [] + + # Increment this a lot in different tasks + for _ in range(100): + task = asyncio.create_task( + self.cog.redis.increment("test_key", 1) + ) + tasks.append(task) + await asyncio.gather(*tasks) + + # Confirm that the value has been incremented the exact right number of times. + value = await self.cog.redis.get("test_key") + self.assertEqual(value, 100) async def test_exceptions_raised(self): """Testing that the various RuntimeErrors are reachable.""" -- cgit v1.2.3 From 4db313e9a7899666f1597094b0d88447c7b64311 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 27 May 2020 20:15:19 +0200 Subject: Floats are no longer permitted as RedisCache keys. Also added a test for this. This is the DRYest approach I could find. It's a little ugly, but I think it's probably good enough. --- bot/utils/redis_cache.py | 116 ++++++++++++++++++++++++------------ tests/bot/utils/test_redis_cache.py | 13 ++-- 2 files changed, 86 insertions(+), 43 deletions(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 33e5d5852..afd37f8f8 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -2,26 +2,42 @@ from __future__ import annotations import asyncio import logging -from typing import Any, Dict, ItemsView, Optional, Union +import typing +from typing import Any, Dict, ItemsView, Optional, Tuple, Union from bot.bot import Bot log = logging.getLogger(__name__) -RedisType = Union[str, int, float] -TYPESTRING_PREFIXES = ( +# Type aliases +RedisKeyType = Union[str, int] +RedisValueType = Union[str, int, float] + +# Prefix tuples +PrefixTuple = Tuple[Tuple[str, Any]] +TYPESTRING_VALUE_PREFIXES = ( ("f|", float), ("i|", int), ("s|", str), ) +TYPESTRING_KEY_PREFIXES = ( + ("i|", int), + ("s|", str), +) # Makes a nice list like "float, int, and str" -NICE_TYPE_LIST = ", ".join(str(_type.__name__) for _, _type in TYPESTRING_PREFIXES) -NICE_TYPE_LIST = ", and ".join(NICE_TYPE_LIST.rsplit(", ", 1)) +NICE_VALUE_TYPE_LIST = ", ".join(str(_type.__name__) for _type in typing.get_args(RedisValueType)) +NICE_VALUE_TYPE_LIST = ", and ".join(NICE_VALUE_TYPE_LIST.rsplit(", ", 1)) + +NICE_KEY_TYPE_LIST = ", ".join(str(_type.__name__) for _type in typing.get_args(RedisKeyType)) +NICE_KEY_TYPE_LIST = ", and ".join(NICE_KEY_TYPE_LIST.rsplit(", ", 1)) # Makes a list like "'f|', 'i|', and 's|'" -NICE_PREFIX_LIST = ", ".join([f"'{prefix}'" for prefix, _ in TYPESTRING_PREFIXES]) -NICE_PREFIX_LIST = ", and ".join(NICE_PREFIX_LIST.rsplit(", ", 1)) +NICE_VALUE_PREFIX_LIST = ", ".join([f"'{prefix}'" for prefix, _ in TYPESTRING_VALUE_PREFIXES]) +NICE_VALUE_PREFIX_LIST = ", and ".join(NICE_VALUE_PREFIX_LIST.rsplit(", ", 1)) + +NICE_KEY_PREFIX_LIST = ", ".join([f"'{prefix}'" for prefix, _ in TYPESTRING_KEY_PREFIXES]) +NICE_KEY_PREFIX_LIST = ", and ".join(NICE_KEY_PREFIX_LIST.rsplit(", ", 1)) class RedisCache: @@ -99,33 +115,57 @@ class RedisCache: self._namespace = namespace @staticmethod - def _to_typestring(value: RedisType) -> str: + def _to_typestring( + key_or_value: Union[RedisKeyType, RedisValueType], + prefixes: PrefixTuple, + nice_type_list: str + ) -> str: """Turn a valid Redis type into a typestring.""" - for prefix, _type in TYPESTRING_PREFIXES: - if isinstance(value, _type): - return f"{prefix}{value}" - raise TypeError(f"RedisCache._from_typestring only supports the types {NICE_TYPE_LIST}.") + for prefix, _type in prefixes: + if isinstance(key_or_value, _type): + return f"{prefix}{key_or_value}" + raise TypeError(f"RedisCache._from_typestring only supports the types {nice_type_list}.") @staticmethod - def _from_typestring(value: Union[bytes, str]) -> RedisType: - """Turn a typestring into a valid Redis type.""" + def _from_typestring( + key_or_value: Union[bytes, str], + prefixes: PrefixTuple, + nice_prefix_list: str, + ) -> Union[RedisKeyType, RedisValueType]: + """Deserialize a typestring into a valid Redis type.""" # Stuff that comes out of Redis will be bytestrings, so let's decode those. - if isinstance(value, bytes): - value = value.decode('utf-8') + if isinstance(key_or_value, bytes): + key_or_value = key_or_value.decode('utf-8') # Now we convert our unicode string back into the type it originally was. - for prefix, _type in TYPESTRING_PREFIXES: - if value.startswith(prefix): - return _type(value[len(prefix):]) - raise TypeError(f"RedisCache._to_typestring only supports the prefixes {NICE_PREFIX_LIST}.") + for prefix, _type in prefixes: + if key_or_value.startswith(prefix): + return _type(key_or_value[len(prefix):]) + raise TypeError(f"RedisCache._to_typestring only supports the prefixes {nice_prefix_list}.") + + def _key_to_typestring(self, key: RedisKeyType) -> str: + """Serialize a RedisKeyType object into a typestring.""" + return self._to_typestring(key, TYPESTRING_KEY_PREFIXES, NICE_KEY_TYPE_LIST) + + def _value_to_typestring(self, value: RedisValueType) -> str: + """Serialize a RedisValueType object into a typestring.""" + return self._to_typestring(value, TYPESTRING_VALUE_PREFIXES, NICE_VALUE_TYPE_LIST) + + def _key_from_typestring(self, key: Union[bytes, str]) -> RedisKeyType: + """Deserialize a RedisKeyType object from a typestring.""" + return self._from_typestring(key, TYPESTRING_KEY_PREFIXES, NICE_KEY_PREFIX_LIST) + + def _value_from_typestring(self, value: Union[bytes, str]) -> RedisValueType: + """Deserialize a RedisValueType object from a typestring.""" + return self._from_typestring(value, TYPESTRING_VALUE_PREFIXES, NICE_VALUE_PREFIX_LIST) def _dict_from_typestring(self, dictionary: Dict) -> Dict: """Turns all contents of a dict into valid Redis types.""" - return {self._from_typestring(key): self._from_typestring(value) for key, value in dictionary.items()} + return {self._key_from_typestring(key): self._value_from_typestring(value) for key, value in dictionary.items()} def _dict_to_typestring(self, dictionary: Dict) -> Dict: """Turns all contents of a dict into typestrings.""" - return {self._to_typestring(key): self._to_typestring(value) for key, value in dictionary.items()} + return {self._key_to_typestring(key): self._value_to_typestring(value) for key, value in dictionary.items()} async def _validate_cache(self) -> None: """Validate that the RedisCache is ready to be used.""" @@ -209,21 +249,21 @@ class RedisCache: """Return a beautiful representation of this object instance.""" return f"RedisCache(namespace={self._namespace!r})" - async def set(self, key: RedisType, value: RedisType) -> None: + async def set(self, key: RedisKeyType, value: RedisValueType) -> None: """Store an item in the Redis cache.""" await self._validate_cache() # Convert to a typestring and then set it - key = self._to_typestring(key) - value = self._to_typestring(value) + key = self._key_to_typestring(key) + value = self._value_to_typestring(value) log.trace(f"Setting {key} to {value}.") await self._redis.hset(self._namespace, key, value) - async def get(self, key: RedisType, default: Optional[RedisType] = None) -> Optional[RedisType]: + async def get(self, key: RedisKeyType, default: Optional[RedisValueType] = None) -> Optional[RedisValueType]: """Get an item from the Redis cache.""" await self._validate_cache() - key = self._to_typestring(key) + key = self._key_to_typestring(key) log.trace(f"Attempting to retrieve {key}.") value = await self._redis.hget(self._namespace, key) @@ -232,11 +272,11 @@ class RedisCache: log.trace(f"Value not found, returning default value {default}") return default else: - value = self._from_typestring(value) + value = self._value_from_typestring(value) log.trace(f"Value found, returning value {value}") return value - async def delete(self, key: RedisType) -> None: + async def delete(self, key: RedisKeyType) -> None: """ Delete an item from the Redis cache. @@ -245,19 +285,19 @@ class RedisCache: See https://redis.io/commands/hdel for more info on how this works. """ await self._validate_cache() - key = self._to_typestring(key) + key = self._key_to_typestring(key) log.trace(f"Attempting to delete {key}.") return await self._redis.hdel(self._namespace, key) - async def contains(self, key: RedisType) -> bool: + async def contains(self, key: RedisKeyType) -> bool: """ Check if a key exists in the Redis cache. Return True if the key exists, otherwise False. """ await self._validate_cache() - key = self._to_typestring(key) + key = self._key_to_typestring(key) exists = await self._redis.hexists(self._namespace, key) log.trace(f"Testing if {key} exists in the RedisCache - Result is {exists}") @@ -304,7 +344,7 @@ class RedisCache: log.trace("Clearing the cache of all key/value pairs.") await self._redis.delete(self._namespace) - async def pop(self, key: RedisType, default: Optional[RedisType] = None) -> RedisType: + async def pop(self, key: RedisKeyType, default: Optional[RedisValueType] = None) -> RedisValueType: """Get the item, remove it from the cache, and provide a default if not found.""" log.trace(f"Attempting to pop {key}.") value = await self.get(key, default) @@ -317,7 +357,7 @@ class RedisCache: return value - async def update(self, items: Dict[RedisType, RedisType]) -> None: + async def update(self, items: Dict[RedisKeyType, RedisValueType]) -> None: """ Update the Redis cache with multiple values. @@ -326,14 +366,14 @@ class RedisCache: do not exist in the RedisCache, they are created. If they do exist, the values are updated with the new ones from `items`. - Please note that both the keys and the values in the `items` dictionary - must consist of valid RedisTypes - ints, floats, or strings. + Please note that keys and the values in the `items` dictionary + must consist of valid RedisKeyTypes and RedisValueTypes. """ await self._validate_cache() log.trace(f"Updating the cache with the following items:\n{items}") await self._redis.hmset_dict(self._namespace, self._dict_to_typestring(items)) - async def increment(self, key: RedisType, amount: Optional[int, float] = 1) -> None: + async def increment(self, key: RedisKeyType, amount: Optional[int, float] = 1) -> None: """ Increment the value by `amount`. @@ -373,7 +413,7 @@ class RedisCache: log.error(error_message) raise TypeError(error_message) - async def decrement(self, key: RedisType, amount: Optional[int, float] = 1) -> None: + async def decrement(self, key: RedisKeyType, amount: Optional[int, float] = 1) -> None: """ Decrement the value by `amount`. diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index efd168dac..4f95dff03 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -70,12 +70,15 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(await self.cog.redis.get('favorite_nothing', "bearclaw"), "bearclaw") async def test_set_item_type(self): - """Test that .set rejects keys and values that are not strings, ints or floats.""" + """Test that .set rejects keys and values that are not permitted.""" fruits = ["lemon", "melon", "apple"] with self.assertRaises(TypeError): await self.cog.redis.set(fruits, "nice") + with self.assertRaises(TypeError): + await self.cog.redis.set(4.23, "nice") + async def test_delete_item(self): """Test that .delete allows us to delete stuff from the RedisCache.""" # Add an item and verify that it gets added @@ -176,16 +179,16 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): # Test conversion to typestring for _input, expected in conversion_tests: - self.assertEqual(self.cog.redis._to_typestring(_input), expected) + self.assertEqual(self.cog.redis._value_to_typestring(_input), expected) # Test conversion from typestrings for _input, expected in conversion_tests: - self.assertEqual(self.cog.redis._from_typestring(expected), _input) + self.assertEqual(self.cog.redis._value_from_typestring(expected), _input) # Test that exceptions are raised on invalid input with self.assertRaises(TypeError): - self.cog.redis._to_typestring(["internet"]) - self.cog.redis._from_typestring("o|firedog") + self.cog.redis._value_to_typestring(["internet"]) + self.cog.redis._value_from_typestring("o|firedog") async def test_increment_decrement(self): """Test .increment and .decrement methods.""" -- cgit v1.2.3 From f66a63501fe1ef8fb5390dfbe42ae9f95ea2bc28 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Thu, 28 May 2020 01:29:34 +0200 Subject: Add custom exceptions for each error state. The bot can get into trouble in three distinct ways: - It has no Bot instance - It has no namespace - It has no parent instance. These happen only if you're using it wrong. To make the test more precise, and to add a little bit more readability (RuntimeError could be anything!), we'll introduce some custom exceptions for these three states. This addresses a review comment by @aeros. --- bot/utils/redis_cache.py | 22 +++++++++++++++++----- tests/bot/utils/test_redis_cache.py | 7 ++++--- 2 files changed, 21 insertions(+), 8 deletions(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index 979ea5d47..6b3c68979 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -27,6 +27,18 @@ _KEY_PREFIXES = ( ) +class NoBotInstanceError(RuntimeError): + """Raised when RedisCache is created without an available bot instance on the owner class.""" + + +class NoNamespaceError(RuntimeError): + """Raised when RedisCache has no namespace, for example if it is not assigned to a class attribute.""" + + +class NoParentInstanceError(RuntimeError): + """Raised when the parent instance is available, for example if called by accessing the parent class directly.""" + + class RedisCache: """ A simplified interface for a Redis connection. @@ -149,7 +161,7 @@ class RedisCache: "This object must be initialized as a class attribute." ) log.error(error_message) - raise RuntimeError(error_message) + raise NoNamespaceError(error_message) if self.bot is None: error_message = ( @@ -159,7 +171,7 @@ class RedisCache: "the RedisCache inside a class that has a Bot instance attribute." ) log.error(error_message) - raise RuntimeError(error_message) + raise NoBotInstanceError(error_message) await self.bot.redis_ready.wait() @@ -194,7 +206,7 @@ class RedisCache: if self._namespace is None: error_message = "RedisCache must be a class attribute." log.error(error_message) - raise RuntimeError(error_message) + raise NoNamespaceError(error_message) if instance is None: error_message = ( @@ -202,7 +214,7 @@ class RedisCache: "before accessing it using the cog's class object." ) log.error(error_message) - raise RuntimeError(error_message) + raise NoParentInstanceError(error_message) for attribute in vars(instance).values(): if isinstance(attribute, Bot): @@ -217,7 +229,7 @@ class RedisCache: "the RedisCache inside a class that has a Bot instance attribute." ) log.error(error_message) - raise RuntimeError(error_message) + raise NoBotInstanceError(error_message) def __repr__(self) -> str: """Return a beautiful representation of this object instance.""" diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 4f95dff03..8c1a40640 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -4,6 +4,7 @@ import unittest import fakeredis.aioredis from bot.utils import RedisCache +from bot.utils.redis_cache import NoBotInstanceError, NoNamespaceError, NoParentInstanceError from tests import helpers @@ -260,13 +261,13 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): cog = MyCog() # Raises "No Bot instance" - with self.assertRaises(RuntimeError): + with self.assertRaises(NoBotInstanceError): await cog.cache.get("john") # Raises "RedisCache has no namespace" - with self.assertRaises(RuntimeError): + with self.assertRaises(NoNamespaceError): await cog.other_cache.get("was") # Raises "You must access the RedisCache instance through the cog instance" - with self.assertRaises(RuntimeError): + with self.assertRaises(NoParentInstanceError): await MyCog.cache.get("afraid") -- cgit v1.2.3