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/helpers.py') 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/helpers.py') 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/helpers.py') 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/helpers.py') 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 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/helpers.py') 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 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/helpers.py') 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