From 085decd12867f89a0803806928741fe6dd3c76bb Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Wed, 15 Apr 2020 08:18:19 +0300 Subject: (Test Helpers): Added `__ge__` function to `MockRole` for comparing. --- tests/helpers.py | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 8e13f0f28..227bac95f 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -205,6 +205,10 @@ class MockRole(CustomMockMixin, unittest.mock.Mock, ColourMixin, HashableMixin): """Simplified position-based comparisons similar to those of `discord.Role`.""" return self.position < other.position + def __ge__(self, other): + """Simplified position-based comparisons similar to those of `discord.Role`.""" + return self.position >= other.position + # Create a Member instance to get a realistic Mock of `discord.Member` member_data = {'user': 'lemon', 'roles': [1]} -- cgit v1.2.3 From 81f6efc2f4e9e157e2f7fb9f191ea410af066632 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 16 Apr 2020 11:15:16 +0300 Subject: (Infraction Tests): Created reason shortening tests for ban and kick. --- tests/bot/cogs/moderation/test_infractions.py | 54 +++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 tests/bot/cogs/moderation/test_infractions.py (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py new file mode 100644 index 000000000..39ea93952 --- /dev/null +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -0,0 +1,54 @@ +import textwrap +import unittest +from unittest.mock import AsyncMock, Mock, patch + +from bot.cogs.moderation.infractions import Infractions +from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole + + +class ShorteningTests(unittest.IsolatedAsyncioTestCase): + """Tests for ban and kick command reason shortening.""" + + def setUp(self): + self.bot = MockBot() + self.cog = Infractions(self.bot) + self.user = MockMember(id=1234, top_role=MockRole(id=3577, position=10)) + self.target = MockMember(id=1265, top_role=MockRole(id=9876, position=0)) + self.guild = MockGuild(id=4567) + self.ctx = MockContext(bot=self.bot, author=self.user, guild=self.guild) + + @patch("bot.cogs.moderation.utils.has_active_infraction") + @patch("bot.cogs.moderation.utils.post_infraction") + async def test_apply_ban_reason_shortening(self, post_infraction_mock, has_active_mock): + """Should truncate reason for `ctx.guild.ban`.""" + has_active_mock.return_value = False + post_infraction_mock.return_value = {"foo": "bar"} + + self.cog.apply_infraction = AsyncMock() + self.bot.get_cog.return_value = AsyncMock() + self.cog.mod_log.ignore = Mock() + + await self.cog.apply_ban(self.ctx, self.target, "foo bar" * 3000) + ban = self.cog.apply_infraction.call_args[0][3] + self.assertEqual( + ban.cr_frame.f_locals["kwargs"]["reason"], + textwrap.shorten("foo bar" * 3000, 512, placeholder=" ...") + ) + # Await ban to avoid warning + await ban + + @patch("bot.cogs.moderation.utils.post_infraction") + async def test_apply_kick_reason_shortening(self, post_infraction_mock) -> None: + """Should truncate reason for `Member.kick`.""" + post_infraction_mock.return_value = {"foo": "bar"} + + self.cog.apply_infraction = AsyncMock() + self.cog.mod_log.ignore = Mock() + + await self.cog.apply_kick(self.ctx, self.target, "foo bar" * 3000) + kick = self.cog.apply_infraction.call_args[0][3] + self.assertEqual( + kick.cr_frame.f_locals["kwargs"]["reason"], + textwrap.shorten("foo bar" * 3000, 512, placeholder="...") + ) + await kick -- cgit v1.2.3 From 216953044a870f2440fe44fcd2f9ca3ee7cf37e9 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 16 Apr 2020 11:30:09 +0300 Subject: (ModLog Tests): Created reason shortening tests for `send_log_message`. --- tests/bot/cogs/moderation/test_modlog.py | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 tests/bot/cogs/moderation/test_modlog.py (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_modlog.py b/tests/bot/cogs/moderation/test_modlog.py new file mode 100644 index 000000000..46e01d2ea --- /dev/null +++ b/tests/bot/cogs/moderation/test_modlog.py @@ -0,0 +1,29 @@ +import unittest + +import discord + +from bot.cogs.moderation.modlog import ModLog +from tests.helpers import MockBot, MockTextChannel + + +class ModLogTests(unittest.IsolatedAsyncioTestCase): + """Tests for moderation logs.""" + + def setUp(self): + self.bot = MockBot() + self.cog = ModLog(self.bot) + self.channel = MockTextChannel() + + async def test_log_entry_description_shortening(self): + """Should truncate embed description for ModLog entry.""" + self.bot.get_channel.return_value = self.channel + await self.cog.send_log_message( + icon_url="foo", + colour=discord.Colour.blue(), + title="bar", + text="foo bar" * 3000 + ) + embed = self.channel.send.call_args[1]["embed"] + self.assertEqual( + embed.description, ("foo bar" * 3000)[:2046] + "..." + ) -- cgit v1.2.3 From 1a3fa6a395141c4fcdd1d388d6ce3e7bd89bcbf0 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 16 Apr 2020 13:40:47 +0300 Subject: (Infractions and ModLog Tests): Replaced `shortening` with `truncation`, removed unnecessary type hint and added comment to kick truncation test about awaiting `kick`. --- tests/bot/cogs/moderation/test_infractions.py | 9 +++++---- tests/bot/cogs/moderation/test_modlog.py | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py index 39ea93952..51a8cc645 100644 --- a/tests/bot/cogs/moderation/test_infractions.py +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -6,8 +6,8 @@ from bot.cogs.moderation.infractions import Infractions from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole -class ShorteningTests(unittest.IsolatedAsyncioTestCase): - """Tests for ban and kick command reason shortening.""" +class TruncationTests(unittest.IsolatedAsyncioTestCase): + """Tests for ban and kick command reason truncation.""" def setUp(self): self.bot = MockBot() @@ -19,7 +19,7 @@ class ShorteningTests(unittest.IsolatedAsyncioTestCase): @patch("bot.cogs.moderation.utils.has_active_infraction") @patch("bot.cogs.moderation.utils.post_infraction") - async def test_apply_ban_reason_shortening(self, post_infraction_mock, has_active_mock): + async def test_apply_ban_reason_truncation(self, post_infraction_mock, has_active_mock): """Should truncate reason for `ctx.guild.ban`.""" has_active_mock.return_value = False post_infraction_mock.return_value = {"foo": "bar"} @@ -38,7 +38,7 @@ class ShorteningTests(unittest.IsolatedAsyncioTestCase): await ban @patch("bot.cogs.moderation.utils.post_infraction") - async def test_apply_kick_reason_shortening(self, post_infraction_mock) -> None: + async def test_apply_kick_reason_truncation(self, post_infraction_mock): """Should truncate reason for `Member.kick`.""" post_infraction_mock.return_value = {"foo": "bar"} @@ -51,4 +51,5 @@ class ShorteningTests(unittest.IsolatedAsyncioTestCase): kick.cr_frame.f_locals["kwargs"]["reason"], textwrap.shorten("foo bar" * 3000, 512, placeholder="...") ) + # Await kick to avoid warning await kick diff --git a/tests/bot/cogs/moderation/test_modlog.py b/tests/bot/cogs/moderation/test_modlog.py index 46e01d2ea..d60836474 100644 --- a/tests/bot/cogs/moderation/test_modlog.py +++ b/tests/bot/cogs/moderation/test_modlog.py @@ -14,7 +14,7 @@ class ModLogTests(unittest.IsolatedAsyncioTestCase): self.cog = ModLog(self.bot) self.channel = MockTextChannel() - async def test_log_entry_description_shortening(self): + async def test_log_entry_description_truncation(self): """Should truncate embed description for ModLog entry.""" self.bot.get_channel.return_value = self.channel await self.cog.send_log_message( -- cgit v1.2.3 From 96920935f9af6d325a2ff91d197285204b3221c9 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 28 Apr 2020 15:56:15 -0700 Subject: Test for out of range datetime in the Duration converter --- tests/bot/test_converters.py | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'tests') diff --git a/tests/bot/test_converters.py b/tests/bot/test_converters.py index ca8cb6825..e42bfc7ee 100644 --- a/tests/bot/test_converters.py +++ b/tests/bot/test_converters.py @@ -198,6 +198,17 @@ class ConverterTests(unittest.TestCase): with self.assertRaises(BadArgument, msg=exception_message): asyncio.run(converter.convert(self.context, invalid_duration)) + @patch("bot.converters.datetime") + def test_duration_converter_out_of_range(self, mock_datetime): + """Duration converter should raise BadArgument if datetime raises a ValueError.""" + mock_datetime.__add__.side_effect = ValueError + mock_datetime.utcnow.return_value = mock_datetime + + duration = f"{datetime.MAXYEAR}y" + exception_message = f"`{duration}` results in a datetime outside the supported range." + with self.assertRaisesRegex(BadArgument, exception_message): + asyncio.run(Duration().convert(self.context, duration)) + def test_isodatetime_converter_for_valid(self): """ISODateTime converter returns correct datetime for valid datetime string.""" test_values = ( -- cgit v1.2.3 From 298389f57166fb5c775e550175c8bb2685fa37ae Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 28 Apr 2020 15:58:29 -0700 Subject: Remove redundant parenthesis from test values --- tests/bot/test_converters.py | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) (limited to 'tests') diff --git a/tests/bot/test_converters.py b/tests/bot/test_converters.py index e42bfc7ee..51d7affba 100644 --- a/tests/bot/test_converters.py +++ b/tests/bot/test_converters.py @@ -166,28 +166,28 @@ class ConverterTests(unittest.TestCase): """Duration raises the right exception for invalid duration strings.""" test_values = ( # Units in wrong order - ('1d1w'), - ('1s1y'), + '1d1w', + '1s1y', # Duplicated units - ('1 year 2 years'), - ('1 M 10 minutes'), + '1 year 2 years', + '1 M 10 minutes', # Unknown substrings - ('1MVes'), - ('1y3breads'), + '1MVes', + '1y3breads', # Missing amount - ('ym'), + 'ym', # Incorrect whitespace - (" 1y"), - ("1S "), - ("1y 1m"), + " 1y", + "1S ", + "1y 1m", # Garbage - ('Guido van Rossum'), - ('lemon lemon lemon lemon lemon lemon lemon'), + 'Guido van Rossum', + 'lemon lemon lemon lemon lemon lemon lemon', ) converter = Duration() @@ -262,19 +262,19 @@ class ConverterTests(unittest.TestCase): """ISODateTime converter raises the correct exception for invalid datetime strings.""" test_values = ( # Make sure it doesn't interfere with the Duration converter - ('1Y'), - ('1d'), - ('1H'), + '1Y', + '1d', + '1H', # Check if it fails when only providing the optional time part - ('10:10:10'), - ('10:00'), + '10:10:10', + '10:00', # Invalid date format - ('19-01-01'), + '19-01-01', # Other non-valid strings - ('fisk the tag master'), + 'fisk the tag master', ) converter = ISODateTime() -- cgit v1.2.3 From 837bc230976328df8dabdc6e8be90188b2ff2ff3 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 28 Apr 2020 16:01:22 -0700 Subject: Use await instead of asyncio.run in converter tests --- tests/bot/test_converters.py | 55 ++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 28 deletions(-) (limited to 'tests') diff --git a/tests/bot/test_converters.py b/tests/bot/test_converters.py index 51d7affba..146a8b5fa 100644 --- a/tests/bot/test_converters.py +++ b/tests/bot/test_converters.py @@ -1,4 +1,3 @@ -import asyncio import datetime import unittest from unittest.mock import MagicMock, patch @@ -16,7 +15,7 @@ from bot.converters import ( ) -class ConverterTests(unittest.TestCase): +class ConverterTests(unittest.IsolatedAsyncioTestCase): """Tests our custom argument converters.""" @classmethod @@ -26,7 +25,7 @@ class ConverterTests(unittest.TestCase): cls.fixed_utc_now = datetime.datetime.fromisoformat('2019-01-01T00:00:00') - def test_tag_content_converter_for_valid(self): + async def test_tag_content_converter_for_valid(self): """TagContentConverter should return correct values for valid input.""" test_values = ( ('hello', 'hello'), @@ -35,10 +34,10 @@ class ConverterTests(unittest.TestCase): for content, expected_conversion in test_values: with self.subTest(content=content, expected_conversion=expected_conversion): - conversion = asyncio.run(TagContentConverter.convert(self.context, content)) + conversion = await TagContentConverter.convert(self.context, content) self.assertEqual(conversion, expected_conversion) - def test_tag_content_converter_for_invalid(self): + async def test_tag_content_converter_for_invalid(self): """TagContentConverter should raise the proper exception for invalid input.""" test_values = ( ('', "Tag contents should not be empty, or filled with whitespace."), @@ -48,9 +47,9 @@ class ConverterTests(unittest.TestCase): for value, exception_message in test_values: with self.subTest(tag_content=value, exception_message=exception_message): with self.assertRaises(BadArgument, msg=exception_message): - asyncio.run(TagContentConverter.convert(self.context, value)) + await TagContentConverter.convert(self.context, value) - def test_tag_name_converter_for_valid(self): + async def test_tag_name_converter_for_valid(self): """TagNameConverter should return the correct values for valid tag names.""" test_values = ( ('tracebacks', 'tracebacks'), @@ -60,10 +59,10 @@ class ConverterTests(unittest.TestCase): for name, expected_conversion in test_values: with self.subTest(name=name, expected_conversion=expected_conversion): - conversion = asyncio.run(TagNameConverter.convert(self.context, name)) + conversion = await TagNameConverter.convert(self.context, name) self.assertEqual(conversion, expected_conversion) - def test_tag_name_converter_for_invalid(self): + async def test_tag_name_converter_for_invalid(self): """TagNameConverter should raise the correct exception for invalid tag names.""" test_values = ( ('👋', "Don't be ridiculous, you can't use that character!"), @@ -76,18 +75,18 @@ class ConverterTests(unittest.TestCase): for invalid_name, exception_message in test_values: with self.subTest(invalid_name=invalid_name, exception_message=exception_message): with self.assertRaises(BadArgument, msg=exception_message): - asyncio.run(TagNameConverter.convert(self.context, invalid_name)) + await TagNameConverter.convert(self.context, invalid_name) - def test_valid_python_identifier_for_valid(self): + async def test_valid_python_identifier_for_valid(self): """ValidPythonIdentifier returns valid identifiers unchanged.""" test_values = ('foo', 'lemon') for name in test_values: with self.subTest(identifier=name): - conversion = asyncio.run(ValidPythonIdentifier.convert(self.context, name)) + conversion = await ValidPythonIdentifier.convert(self.context, name) self.assertEqual(name, conversion) - def test_valid_python_identifier_for_invalid(self): + async def test_valid_python_identifier_for_invalid(self): """ValidPythonIdentifier raises the proper exception for invalid identifiers.""" test_values = ('nested.stuff', '#####') @@ -95,9 +94,9 @@ class ConverterTests(unittest.TestCase): with self.subTest(identifier=name): exception_message = f'`{name}` is not a valid Python identifier' with self.assertRaises(BadArgument, msg=exception_message): - asyncio.run(ValidPythonIdentifier.convert(self.context, name)) + await ValidPythonIdentifier.convert(self.context, name) - def test_duration_converter_for_valid(self): + async def test_duration_converter_for_valid(self): """Duration returns the correct `datetime` for valid duration strings.""" test_values = ( # Simple duration strings @@ -159,10 +158,10 @@ class ConverterTests(unittest.TestCase): mock_datetime.utcnow.return_value = self.fixed_utc_now with self.subTest(duration=duration, duration_dict=duration_dict): - converted_datetime = asyncio.run(converter.convert(self.context, duration)) + converted_datetime = await converter.convert(self.context, duration) self.assertEqual(converted_datetime, expected_datetime) - def test_duration_converter_for_invalid(self): + async def test_duration_converter_for_invalid(self): """Duration raises the right exception for invalid duration strings.""" test_values = ( # Units in wrong order @@ -196,10 +195,10 @@ class ConverterTests(unittest.TestCase): with self.subTest(invalid_duration=invalid_duration): exception_message = f'`{invalid_duration}` is not a valid duration string.' with self.assertRaises(BadArgument, msg=exception_message): - asyncio.run(converter.convert(self.context, invalid_duration)) + await converter.convert(self.context, invalid_duration) @patch("bot.converters.datetime") - def test_duration_converter_out_of_range(self, mock_datetime): + async def test_duration_converter_out_of_range(self, mock_datetime): """Duration converter should raise BadArgument if datetime raises a ValueError.""" mock_datetime.__add__.side_effect = ValueError mock_datetime.utcnow.return_value = mock_datetime @@ -207,9 +206,9 @@ class ConverterTests(unittest.TestCase): duration = f"{datetime.MAXYEAR}y" exception_message = f"`{duration}` results in a datetime outside the supported range." with self.assertRaisesRegex(BadArgument, exception_message): - asyncio.run(Duration().convert(self.context, duration)) + await Duration().convert(self.context, duration) - def test_isodatetime_converter_for_valid(self): + async def test_isodatetime_converter_for_valid(self): """ISODateTime converter returns correct datetime for valid datetime string.""" test_values = ( # `YYYY-mm-ddTHH:MM:SSZ` | `YYYY-mm-dd HH:MM:SSZ` @@ -254,11 +253,11 @@ class ConverterTests(unittest.TestCase): for datetime_string, expected_dt in test_values: with self.subTest(datetime_string=datetime_string, expected_dt=expected_dt): - converted_dt = asyncio.run(converter.convert(self.context, datetime_string)) + converted_dt = await converter.convert(self.context, datetime_string) self.assertIsNone(converted_dt.tzinfo) self.assertEqual(converted_dt, expected_dt) - def test_isodatetime_converter_for_invalid(self): + async def test_isodatetime_converter_for_invalid(self): """ISODateTime converter raises the correct exception for invalid datetime strings.""" test_values = ( # Make sure it doesn't interfere with the Duration converter @@ -282,9 +281,9 @@ class ConverterTests(unittest.TestCase): with self.subTest(datetime_string=datetime_string): exception_message = f"`{datetime_string}` is not a valid ISO-8601 datetime string" with self.assertRaises(BadArgument, msg=exception_message): - asyncio.run(converter.convert(self.context, datetime_string)) + await converter.convert(self.context, datetime_string) - def test_hush_duration_converter_for_valid(self): + async def test_hush_duration_converter_for_valid(self): """HushDurationConverter returns correct value for minutes duration or `"forever"` strings.""" test_values = ( ("0", 0), @@ -297,10 +296,10 @@ class ConverterTests(unittest.TestCase): converter = HushDurationConverter() for minutes_string, expected_minutes in test_values: with self.subTest(minutes_string=minutes_string, expected_minutes=expected_minutes): - converted = asyncio.run(converter.convert(self.context, minutes_string)) + converted = await converter.convert(self.context, minutes_string) self.assertEqual(expected_minutes, converted) - def test_hush_duration_converter_for_invalid(self): + async def test_hush_duration_converter_for_invalid(self): """HushDurationConverter raises correct exception for invalid minutes duration strings.""" test_values = ( ("16", "Duration must be at most 15 minutes."), @@ -311,4 +310,4 @@ class ConverterTests(unittest.TestCase): for invalid_minutes_string, exception_message in test_values: with self.subTest(invalid_minutes_string=invalid_minutes_string, exception_message=exception_message): with self.assertRaisesRegex(BadArgument, exception_message): - asyncio.run(converter.convert(self.context, invalid_minutes_string)) + await converter.convert(self.context, invalid_minutes_string) -- cgit v1.2.3 From 1e4766d9934396a72cc759649049b07e5814004a Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 28 Apr 2020 16:18:34 -0700 Subject: Fix exception message assertions in converter tests The `msg` arg is for displaying a message when the assertion fails. To match against the exception's message, `assertRaisesRegex` must be used. Since all of the messages are meant to be interpreted literally rather than as regex, `re.escape` is used. --- tests/bot/test_converters.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/bot/test_converters.py b/tests/bot/test_converters.py index 146a8b5fa..c42111f3f 100644 --- a/tests/bot/test_converters.py +++ b/tests/bot/test_converters.py @@ -1,4 +1,5 @@ import datetime +import re import unittest from unittest.mock import MagicMock, patch @@ -46,7 +47,7 @@ class ConverterTests(unittest.IsolatedAsyncioTestCase): for value, exception_message in test_values: with self.subTest(tag_content=value, exception_message=exception_message): - with self.assertRaises(BadArgument, msg=exception_message): + with self.assertRaisesRegex(BadArgument, re.escape(exception_message)): await TagContentConverter.convert(self.context, value) async def test_tag_name_converter_for_valid(self): @@ -74,7 +75,7 @@ class ConverterTests(unittest.IsolatedAsyncioTestCase): for invalid_name, exception_message in test_values: with self.subTest(invalid_name=invalid_name, exception_message=exception_message): - with self.assertRaises(BadArgument, msg=exception_message): + with self.assertRaisesRegex(BadArgument, re.escape(exception_message)): await TagNameConverter.convert(self.context, invalid_name) async def test_valid_python_identifier_for_valid(self): @@ -93,7 +94,7 @@ class ConverterTests(unittest.IsolatedAsyncioTestCase): for name in test_values: with self.subTest(identifier=name): exception_message = f'`{name}` is not a valid Python identifier' - with self.assertRaises(BadArgument, msg=exception_message): + with self.assertRaisesRegex(BadArgument, re.escape(exception_message)): await ValidPythonIdentifier.convert(self.context, name) async def test_duration_converter_for_valid(self): @@ -194,7 +195,7 @@ class ConverterTests(unittest.IsolatedAsyncioTestCase): for invalid_duration in test_values: with self.subTest(invalid_duration=invalid_duration): exception_message = f'`{invalid_duration}` is not a valid duration string.' - with self.assertRaises(BadArgument, msg=exception_message): + with self.assertRaisesRegex(BadArgument, re.escape(exception_message)): await converter.convert(self.context, invalid_duration) @patch("bot.converters.datetime") @@ -205,7 +206,7 @@ class ConverterTests(unittest.IsolatedAsyncioTestCase): duration = f"{datetime.MAXYEAR}y" exception_message = f"`{duration}` results in a datetime outside the supported range." - with self.assertRaisesRegex(BadArgument, exception_message): + with self.assertRaisesRegex(BadArgument, re.escape(exception_message)): await Duration().convert(self.context, duration) async def test_isodatetime_converter_for_valid(self): @@ -280,7 +281,7 @@ class ConverterTests(unittest.IsolatedAsyncioTestCase): for datetime_string in test_values: with self.subTest(datetime_string=datetime_string): exception_message = f"`{datetime_string}` is not a valid ISO-8601 datetime string" - with self.assertRaises(BadArgument, msg=exception_message): + with self.assertRaisesRegex(BadArgument, re.escape(exception_message)): await converter.convert(self.context, datetime_string) async def test_hush_duration_converter_for_valid(self): @@ -309,5 +310,5 @@ class ConverterTests(unittest.IsolatedAsyncioTestCase): converter = HushDurationConverter() for invalid_minutes_string, exception_message in test_values: with self.subTest(invalid_minutes_string=invalid_minutes_string, exception_message=exception_message): - with self.assertRaisesRegex(BadArgument, exception_message): + with self.assertRaisesRegex(BadArgument, re.escape(exception_message)): await converter.convert(self.context, invalid_minutes_string) -- cgit v1.2.3 From 601ff03823deb842d74f4689fecb68f7ce1693e6 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 18:11:44 +0200 Subject: AntiMalware Tests - Added unittest for message without attachment --- tests/bot/cogs/test_antimalware.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/bot/cogs/test_antimalware.py (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py new file mode 100644 index 000000000..41ca19e17 --- /dev/null +++ b/tests/bot/cogs/test_antimalware.py @@ -0,0 +1,20 @@ +import asyncio +import unittest + +from bot.cogs import antimalware +from tests.helpers import MockBot, MockMessage + + +class AntiMalwareCogTests(unittest.TestCase): + """Test the AntiMalware cog.""" + + def setUp(self): + """Sets up fresh objects for each test.""" + self.bot = MockBot() + self.cog = antimalware.AntiMalware(self.bot) + self.message = MockMessage() + + def test_message_without_attachment(self): + """Messages without attachments should result in no action.""" + coroutine = self.cog.on_message(self.message) + self.assertIsNone(asyncio.run(coroutine)) -- cgit v1.2.3 From 9889f0fdd1ba403ae50ba20be38feca0932d1dda Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 19:06:46 +0200 Subject: AntiMalware Tests - Added unittests for deletion of message and ignoring of dms --- tests/bot/cogs/test_antimalware.py | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 41ca19e17..ebf3a1277 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -1,8 +1,9 @@ import asyncio import unittest +from unittest.mock import AsyncMock from bot.cogs import antimalware -from tests.helpers import MockBot, MockMessage +from tests.helpers import MockAttachment, MockBot, MockMessage class AntiMalwareCogTests(unittest.TestCase): @@ -13,8 +14,27 @@ class AntiMalwareCogTests(unittest.TestCase): self.bot = MockBot() self.cog = antimalware.AntiMalware(self.bot) self.message = MockMessage() + self.message.delete = AsyncMock() def test_message_without_attachment(self): """Messages without attachments should result in no action.""" coroutine = self.cog.on_message(self.message) self.assertIsNone(asyncio.run(coroutine)) + self.message.delete.assert_not_called() + + def test_direct_message_with_attachment(self): + """Direct messages should have no action taken.""" + attachment = MockAttachment(filename="python.asdfsff") + self.message.attachments = [attachment] + self.message.guild = None + coroutine = self.cog.on_message(self.message) + asyncio.run(coroutine) + self.message.delete.assert_not_called() + + def test_message_with_illegal_extension_gets_deleted(self): + """A message containing an illegal extension should send an embed.""" + attachment = MockAttachment(filename="python.asdfsff") + self.message.attachments = [attachment] + coroutine = self.cog.on_message(self.message) + asyncio.run(coroutine) + self.message.delete.assert_called_once() -- cgit v1.2.3 From 90d2ce0e3717d4ddf79eb986e22f7542ca1770e1 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 19:13:45 +0200 Subject: AntiMalware Tests - Added unittest for messages send by staff --- tests/bot/cogs/test_antimalware.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index ebf3a1277..e3fd477fa 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -3,7 +3,8 @@ import unittest from unittest.mock import AsyncMock from bot.cogs import antimalware -from tests.helpers import MockAttachment, MockBot, MockMessage +from bot.constants import Roles +from tests.helpers import MockAttachment, MockBot, MockMessage, MockRole class AntiMalwareCogTests(unittest.TestCase): @@ -38,3 +39,13 @@ class AntiMalwareCogTests(unittest.TestCase): coroutine = self.cog.on_message(self.message) asyncio.run(coroutine) self.message.delete.assert_called_once() + + def test_message_send_by_staff(self): + """A message send by a member of staff should be ignored.""" + moderator_role = MockRole(name="Moderator", id=Roles.moderators) + self.message.author.roles.append(moderator_role) + attachment = MockAttachment(filename="python.asdfsff") + self.message.attachments = [attachment] + coroutine = self.cog.on_message(self.message) + asyncio.run(coroutine) + self.message.delete.assert_not_called() -- cgit v1.2.3 From 3913a8eba46bf98bd09e13145da33f7a09f77960 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 19:45:57 +0200 Subject: AntiMalware Tests - Added unittest for the embed for a python file. --- tests/bot/cogs/test_antimalware.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index e3fd477fa..0bb5af943 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -3,7 +3,7 @@ import unittest from unittest.mock import AsyncMock from bot.cogs import antimalware -from bot.constants import Roles +from bot.constants import Roles, URLs from tests.helpers import MockAttachment, MockBot, MockMessage, MockRole @@ -28,16 +28,20 @@ class AntiMalwareCogTests(unittest.TestCase): attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] self.message.guild = None + coroutine = self.cog.on_message(self.message) asyncio.run(coroutine) + self.message.delete.assert_not_called() def test_message_with_illegal_extension_gets_deleted(self): """A message containing an illegal extension should send an embed.""" attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] + coroutine = self.cog.on_message(self.message) asyncio.run(coroutine) + self.message.delete.assert_called_once() def test_message_send_by_staff(self): @@ -46,6 +50,25 @@ class AntiMalwareCogTests(unittest.TestCase): self.message.author.roles.append(moderator_role) attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] + coroutine = self.cog.on_message(self.message) asyncio.run(coroutine) + self.message.delete.assert_not_called() + + def test_python_file_redirect_embed(self): + """A message containing a .python file should result in an embed redirecting the user to our paste site""" + attachment = MockAttachment(filename="python.py") + self.message.attachments = [attachment] + self.message.channel.send = AsyncMock() + + coroutine = self.cog.on_message(self.message) + asyncio.run(coroutine) + args, kwargs = self.message.channel.send.call_args + embed = kwargs.pop("embed") + + self.assertEqual(args[0], f"Hey {self.message.author.mention}!") + self.assertEqual(embed.description, ( + "It looks like you tried to attach a Python file - " + f"please use a code-pasting service such as {URLs.site_schema}{URLs.site_paste}" + )) -- cgit v1.2.3 From 19c15d957040b6857a4141e15c32fd0526f9920d Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 20:15:17 +0200 Subject: AntiMalware Tests - Added unittest for messages that were deleted in the meantime. --- tests/bot/cogs/test_antimalware.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 0bb5af943..da5cd9d11 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -1,6 +1,9 @@ import asyncio +import logging import unittest -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, Mock + +from discord import NotFound from bot.cogs import antimalware from bot.constants import Roles, URLs @@ -72,3 +75,18 @@ class AntiMalwareCogTests(unittest.TestCase): "It looks like you tried to attach a Python file - " f"please use a code-pasting service such as {URLs.site_schema}{URLs.site_paste}" )) + + def test_removing_deleted_message_logs(self): + """Removing an already deleted message logs the correct message""" + attachment = MockAttachment(filename="python.py") + self.message.attachments = [attachment] + self.message.delete = AsyncMock(side_effect=NotFound(response=Mock(status=""), message="")) + + coroutine = self.cog.on_message(self.message) + logger = logging.getLogger("bot.cogs.antimalware") + + with self.assertLogs(logger=logger, level="INFO") as logs: + asyncio.run(coroutine) + self.assertIn( + f"INFO:bot.cogs.antimalware:Tried to delete message `{self.message.id}`, but message could not be found.", + logs.output) -- cgit v1.2.3 From 4a0b3ea1ef182ddbbb1f9d731b28768a049a531d Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 20:23:00 +0200 Subject: AntiMalware Tests - Added unittest for cog setup --- tests/bot/cogs/test_antimalware.py | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index da5cd9d11..67c640d23 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -90,3 +90,13 @@ class AntiMalwareCogTests(unittest.TestCase): self.assertIn( f"INFO:bot.cogs.antimalware:Tried to delete message `{self.message.id}`, but message could not be found.", logs.output) + + +class AntiMalwareSetupTests(unittest.TestCase): + """Tests setup of the `AntiMalware` cog.""" + + def test_setup(self): + """Setup of the extension should call add_cog.""" + bot = MockBot() + antimalware.setup(bot) + bot.add_cog.assert_called_once() -- cgit v1.2.3 From 3090141f673279f2836cb3aca95397eb9950ad0f Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 20:41:31 +0200 Subject: AntiMalware Tests - Added unittest message deletion log --- tests/bot/cogs/test_antimalware.py | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 67c640d23..b4e31b5ce 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -1,14 +1,17 @@ import asyncio import logging import unittest +from os.path import splitext from unittest.mock import AsyncMock, Mock from discord import NotFound from bot.cogs import antimalware -from bot.constants import Roles, URLs +from bot.constants import AntiMalware as AntiMalwareConfig, Roles, URLs from tests.helpers import MockAttachment, MockBot, MockMessage, MockRole +MODULE = "bot.cogs.antimalware" + class AntiMalwareCogTests(unittest.TestCase): """Test the AntiMalware cog.""" @@ -78,17 +81,38 @@ class AntiMalwareCogTests(unittest.TestCase): def test_removing_deleted_message_logs(self): """Removing an already deleted message logs the correct message""" - attachment = MockAttachment(filename="python.py") + attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] self.message.delete = AsyncMock(side_effect=NotFound(response=Mock(status=""), message="")) coroutine = self.cog.on_message(self.message) - logger = logging.getLogger("bot.cogs.antimalware") + logger = logging.getLogger(MODULE) with self.assertLogs(logger=logger, level="INFO") as logs: asyncio.run(coroutine) self.assertIn( - f"INFO:bot.cogs.antimalware:Tried to delete message `{self.message.id}`, but message could not be found.", + f"INFO:{MODULE}:Tried to delete message `{self.message.id}`, but message could not be found.", + logs.output) + + def test_message_with_illegal_attachment_logs(self): + """Deleting a message with an illegal attachment should result in a log.""" + attachment = MockAttachment(filename="python.asdfsff") + self.message.attachments = [attachment] + + coroutine = self.cog.on_message(self.message) + file_extensions = {splitext(attachment.filename.lower())[1] for attachment in self.message.attachments} + extensions_blocked = file_extensions - set(AntiMalwareConfig.whitelist) + blocked_extensions_str = ', '.join(extensions_blocked) + logger = logging.getLogger(MODULE) + + with self.assertLogs(logger=logger, level="INFO") as logs: + asyncio.run(coroutine) + self.assertEqual( + [ + f"INFO:{MODULE}:" + f"User '{self.message.author}' ({self.message.author.id}) " + f"uploaded blacklisted file(s): {blocked_extensions_str}" + ], logs.output) -- cgit v1.2.3 From f0bc9d800dd141b9126c48251a80618e138d61f1 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 20:46:15 +0200 Subject: AntiMalware Tests - Added unittest for valid attachment --- tests/bot/cogs/test_antimalware.py | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index b4e31b5ce..407fa05c1 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -23,6 +23,15 @@ class AntiMalwareCogTests(unittest.TestCase): self.message = MockMessage() self.message.delete = AsyncMock() + def test_message_with_allowed_attachment(self): + """Messages with allowed extensions should not be deleted""" + attachment = MockAttachment(filename=f"python.{AntiMalwareConfig.whitelist[0]}") + self.message.attachments = [attachment] + + coroutine = self.cog.on_message(self.message) + asyncio.run(coroutine) + self.message.delete.assert_not_called() + def test_message_without_attachment(self): """Messages without attachments should result in no action.""" coroutine = self.cog.on_message(self.message) -- cgit v1.2.3 From 75f6ca6bd9b695a5deb4a4d78311bc63eb2a74d0 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 21:04:47 +0200 Subject: AntiMalware Tests - Added unittest for txt file attachment --- tests/bot/cogs/test_antimalware.py | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 407fa05c1..eba439afb 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -7,7 +7,7 @@ from unittest.mock import AsyncMock, Mock from discord import NotFound from bot.cogs import antimalware -from bot.constants import AntiMalware as AntiMalwareConfig, Roles, URLs +from bot.constants import AntiMalware as AntiMalwareConfig, Channels, Roles, URLs from tests.helpers import MockAttachment, MockBot, MockMessage, MockRole MODULE = "bot.cogs.antimalware" @@ -21,7 +21,6 @@ class AntiMalwareCogTests(unittest.TestCase): self.bot = MockBot() self.cog = antimalware.AntiMalware(self.bot) self.message = MockMessage() - self.message.delete = AsyncMock() def test_message_with_allowed_attachment(self): """Messages with allowed extensions should not be deleted""" @@ -88,6 +87,28 @@ class AntiMalwareCogTests(unittest.TestCase): f"please use a code-pasting service such as {URLs.site_schema}{URLs.site_paste}" )) + def test_txt_file_redirect_embed(self): + attachment = MockAttachment(filename="python.txt") + self.message.attachments = [attachment] + self.message.channel.send = AsyncMock() + + coroutine = self.cog.on_message(self.message) + asyncio.run(coroutine) + args, kwargs = self.message.channel.send.call_args + embed = kwargs.pop("embed") + cmd_channel = self.bot.get_channel(Channels.bot_commands) + + self.assertEqual(args[0], f"Hey {self.message.author.mention}!") + self.assertEqual(embed.description, ( + "**Uh-oh!** It looks like your message got zapped by our spam filter. " + "We currently don't allow `.txt` attachments, so here are some tips to help you travel safely: \n\n" + "• If you attempted to send a message longer than 2000 characters, try shortening your message " + "to fit within the character limit or use a pasting service (see below) \n\n" + "• If you tried to show someone your code, you can use codeblocks \n(run `!code-blocks` in " + f"{cmd_channel.mention} for more information) or use a pasting service like: " + f"\n\n{URLs.site_schema}{URLs.site_paste}" + )) + def test_removing_deleted_message_logs(self): """Removing an already deleted message logs the correct message""" attachment = MockAttachment(filename="python.asdfsff") -- cgit v1.2.3 From c8bf44e30c286b27768601d5a04cd2459f170d4c Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Thu, 7 May 2020 21:29:15 +0200 Subject: AntiMalware Tests - Switched to unittest.IsolatedAsyncioTestCase --- tests/bot/cogs/test_antimalware.py | 48 +++++++++++++++----------------------- 1 file changed, 19 insertions(+), 29 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index eba439afb..6fb7b399e 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -1,4 +1,3 @@ -import asyncio import logging import unittest from os.path import splitext @@ -13,7 +12,7 @@ from tests.helpers import MockAttachment, MockBot, MockMessage, MockRole MODULE = "bot.cogs.antimalware" -class AntiMalwareCogTests(unittest.TestCase): +class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): """Test the AntiMalware cog.""" def setUp(self): @@ -22,62 +21,56 @@ class AntiMalwareCogTests(unittest.TestCase): self.cog = antimalware.AntiMalware(self.bot) self.message = MockMessage() - def test_message_with_allowed_attachment(self): + async def test_message_with_allowed_attachment(self): """Messages with allowed extensions should not be deleted""" attachment = MockAttachment(filename=f"python.{AntiMalwareConfig.whitelist[0]}") self.message.attachments = [attachment] - coroutine = self.cog.on_message(self.message) - asyncio.run(coroutine) + await self.cog.on_message(self.message) self.message.delete.assert_not_called() - def test_message_without_attachment(self): + async def test_message_without_attachment(self): """Messages without attachments should result in no action.""" - coroutine = self.cog.on_message(self.message) - self.assertIsNone(asyncio.run(coroutine)) + self.assertIsNone(await self.cog.on_message(self.message)) self.message.delete.assert_not_called() - def test_direct_message_with_attachment(self): + async def test_direct_message_with_attachment(self): """Direct messages should have no action taken.""" attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] self.message.guild = None - coroutine = self.cog.on_message(self.message) - asyncio.run(coroutine) + await self.cog.on_message(self.message) self.message.delete.assert_not_called() - def test_message_with_illegal_extension_gets_deleted(self): + async def test_message_with_illegal_extension_gets_deleted(self): """A message containing an illegal extension should send an embed.""" attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] - coroutine = self.cog.on_message(self.message) - asyncio.run(coroutine) + await self.cog.on_message(self.message) self.message.delete.assert_called_once() - def test_message_send_by_staff(self): + async def test_message_send_by_staff(self): """A message send by a member of staff should be ignored.""" moderator_role = MockRole(name="Moderator", id=Roles.moderators) self.message.author.roles.append(moderator_role) attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] - coroutine = self.cog.on_message(self.message) - asyncio.run(coroutine) + await self.cog.on_message(self.message) self.message.delete.assert_not_called() - def test_python_file_redirect_embed(self): + async def test_python_file_redirect_embed(self): """A message containing a .python file should result in an embed redirecting the user to our paste site""" attachment = MockAttachment(filename="python.py") self.message.attachments = [attachment] self.message.channel.send = AsyncMock() - coroutine = self.cog.on_message(self.message) - asyncio.run(coroutine) + await self.cog.on_message(self.message) args, kwargs = self.message.channel.send.call_args embed = kwargs.pop("embed") @@ -87,13 +80,12 @@ class AntiMalwareCogTests(unittest.TestCase): f"please use a code-pasting service such as {URLs.site_schema}{URLs.site_paste}" )) - def test_txt_file_redirect_embed(self): + async def test_txt_file_redirect_embed(self): attachment = MockAttachment(filename="python.txt") self.message.attachments = [attachment] self.message.channel.send = AsyncMock() - coroutine = self.cog.on_message(self.message) - asyncio.run(coroutine) + await self.cog.on_message(self.message) args, kwargs = self.message.channel.send.call_args embed = kwargs.pop("embed") cmd_channel = self.bot.get_channel(Channels.bot_commands) @@ -109,34 +101,32 @@ class AntiMalwareCogTests(unittest.TestCase): f"\n\n{URLs.site_schema}{URLs.site_paste}" )) - def test_removing_deleted_message_logs(self): + async def test_removing_deleted_message_logs(self): """Removing an already deleted message logs the correct message""" attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] self.message.delete = AsyncMock(side_effect=NotFound(response=Mock(status=""), message="")) - coroutine = self.cog.on_message(self.message) logger = logging.getLogger(MODULE) with self.assertLogs(logger=logger, level="INFO") as logs: - asyncio.run(coroutine) + await self.cog.on_message(self.message) self.assertIn( f"INFO:{MODULE}:Tried to delete message `{self.message.id}`, but message could not be found.", logs.output) - def test_message_with_illegal_attachment_logs(self): + async def test_message_with_illegal_attachment_logs(self): """Deleting a message with an illegal attachment should result in a log.""" attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] - coroutine = self.cog.on_message(self.message) file_extensions = {splitext(attachment.filename.lower())[1] for attachment in self.message.attachments} extensions_blocked = file_extensions - set(AntiMalwareConfig.whitelist) blocked_extensions_str = ', '.join(extensions_blocked) logger = logging.getLogger(MODULE) with self.assertLogs(logger=logger, level="INFO") as logs: - asyncio.run(coroutine) + await self.cog.on_message(self.message) self.assertEqual( [ f"INFO:{MODULE}:" -- cgit v1.2.3 From bd9537ba85154ece1dca39ec03d36dd7d39a8388 Mon Sep 17 00:00:00 2001 From: MrGrote Date: Fri, 8 May 2020 22:11:54 +0200 Subject: Update tests/bot/cogs/test_antimalware.py Co-authored-by: Mark --- tests/bot/cogs/test_antimalware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 6fb7b399e..e0aa9d6d2 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -65,7 +65,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.message.delete.assert_not_called() async def test_python_file_redirect_embed(self): - """A message containing a .python file should result in an embed redirecting the user to our paste site""" + """A message containing a .py file should result in an embed redirecting the user to our paste site""" attachment = MockAttachment(filename="python.py") self.message.attachments = [attachment] self.message.channel.send = AsyncMock() -- cgit v1.2.3 From 847a78a76c08a670e85d926e3afa43e1cc3180f4 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Mon, 11 May 2020 19:41:46 +0200 Subject: AntiMalware Tests - implemented minor feedback --- tests/bot/cogs/test_antimalware.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index e0aa9d6d2..6e06df0a8 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -1,4 +1,3 @@ -import logging import unittest from os.path import splitext from unittest.mock import AsyncMock, Mock @@ -6,7 +5,7 @@ from unittest.mock import AsyncMock, Mock from discord import NotFound from bot.cogs import antimalware -from bot.constants import AntiMalware as AntiMalwareConfig, Channels, Roles, URLs +from bot.constants import AntiMalware as AntiMalwareConfig, Channels, STAFF_ROLES, URLs from tests.helpers import MockAttachment, MockBot, MockMessage, MockRole MODULE = "bot.cogs.antimalware" @@ -31,7 +30,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): async def test_message_without_attachment(self): """Messages without attachments should result in no action.""" - self.assertIsNone(await self.cog.on_message(self.message)) + await self.cog.on_message(self.message) self.message.delete.assert_not_called() async def test_direct_message_with_attachment(self): @@ -55,8 +54,8 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): async def test_message_send_by_staff(self): """A message send by a member of staff should be ignored.""" - moderator_role = MockRole(name="Moderator", id=Roles.moderators) - self.message.author.roles.append(moderator_role) + staff_role = MockRole(id=STAFF_ROLES[0]) + self.message.author.roles.append(staff_role) attachment = MockAttachment(filename="python.asdfsff") self.message.attachments = [attachment] @@ -71,6 +70,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.message.channel.send = AsyncMock() await self.cog.on_message(self.message) + self.message.channel.send.assert_called_once() args, kwargs = self.message.channel.send.call_args embed = kwargs.pop("embed") @@ -107,13 +107,13 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.message.attachments = [attachment] self.message.delete = AsyncMock(side_effect=NotFound(response=Mock(status=""), message="")) - logger = logging.getLogger(MODULE) - - with self.assertLogs(logger=logger, level="INFO") as logs: + with self.assertLogs(logger=antimalware.log, level="INFO") as logs: await self.cog.on_message(self.message) + self.message.delete.assert_called_once() self.assertIn( f"INFO:{MODULE}:Tried to delete message `{self.message.id}`, but message could not be found.", - logs.output) + logs.output + ) async def test_message_with_illegal_attachment_logs(self): """Deleting a message with an illegal attachment should result in a log.""" @@ -123,9 +123,8 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): file_extensions = {splitext(attachment.filename.lower())[1] for attachment in self.message.attachments} extensions_blocked = file_extensions - set(AntiMalwareConfig.whitelist) blocked_extensions_str = ', '.join(extensions_blocked) - logger = logging.getLogger(MODULE) - with self.assertLogs(logger=logger, level="INFO") as logs: + with self.assertLogs(logger=antimalware.log, level="INFO") as logs: await self.cog.on_message(self.message) self.assertEqual( [ @@ -133,7 +132,8 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): f"User '{self.message.author}' ({self.message.author.id}) " f"uploaded blacklisted file(s): {blocked_extensions_str}" ], - logs.output) + logs.output + ) class AntiMalwareSetupTests(unittest.TestCase): -- cgit v1.2.3 From ba71ac5b002dd3e1ee6a916ba2705a7cff697a66 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Mon, 11 May 2020 20:24:20 +0200 Subject: AntiMalware Tests - extracted the method for determining disallowed extensions and added a test for it. --- tests/bot/cogs/test_antimalware.py | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 6e06df0a8..78ad996f2 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -19,10 +19,11 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.bot = MockBot() self.cog = antimalware.AntiMalware(self.bot) self.message = MockMessage() + AntiMalwareConfig.whitelist = [".first", ".second", ".third"] async def test_message_with_allowed_attachment(self): """Messages with allowed extensions should not be deleted""" - attachment = MockAttachment(filename=f"python.{AntiMalwareConfig.whitelist[0]}") + attachment = MockAttachment(filename=f"python{AntiMalwareConfig.whitelist[0]}") self.message.attachments = [attachment] await self.cog.on_message(self.message) @@ -35,7 +36,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): async def test_direct_message_with_attachment(self): """Direct messages should have no action taken.""" - attachment = MockAttachment(filename="python.asdfsff") + attachment = MockAttachment(filename="python.disallowed") self.message.attachments = [attachment] self.message.guild = None @@ -45,7 +46,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): async def test_message_with_illegal_extension_gets_deleted(self): """A message containing an illegal extension should send an embed.""" - attachment = MockAttachment(filename="python.asdfsff") + attachment = MockAttachment(filename="python.disallowed") self.message.attachments = [attachment] await self.cog.on_message(self.message) @@ -56,7 +57,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): """A message send by a member of staff should be ignored.""" staff_role = MockRole(id=STAFF_ROLES[0]) self.message.author.roles.append(staff_role) - attachment = MockAttachment(filename="python.asdfsff") + attachment = MockAttachment(filename="python.disallowed") self.message.attachments = [attachment] await self.cog.on_message(self.message) @@ -103,7 +104,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): async def test_removing_deleted_message_logs(self): """Removing an already deleted message logs the correct message""" - attachment = MockAttachment(filename="python.asdfsff") + attachment = MockAttachment(filename="python.disallowed") self.message.attachments = [attachment] self.message.delete = AsyncMock(side_effect=NotFound(response=Mock(status=""), message="")) @@ -117,7 +118,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): async def test_message_with_illegal_attachment_logs(self): """Deleting a message with an illegal attachment should result in a log.""" - attachment = MockAttachment(filename="python.asdfsff") + attachment = MockAttachment(filename="python.disallowed") self.message.attachments = [attachment] file_extensions = {splitext(attachment.filename.lower())[1] for attachment in self.message.attachments} @@ -135,6 +136,22 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): logs.output ) + async def test_get_disallowed_extensions(self): + """The return value should include all non-whitelisted extensions.""" + test_values = ( + (AntiMalwareConfig.whitelist, []), + ([".first"], []), + ([".first", ".disallowed"], [".disallowed"]), + ([".disallowed"], [".disallowed"]), + ([".disallowed", ".illegal"], [".disallowed", ".illegal"]), + ) + + for extensions, expected_disallowed_extensions in test_values: + with self.subTest(extensions=extensions, expected_disallowed_extensions=expected_disallowed_extensions): + self.message.attachments = [MockAttachment(filename=f"filename{extension}") for extension in extensions] + disallowed_extensions = self.cog.get_disallowed_extensions(self.message) + self.assertCountEqual(disallowed_extensions, expected_disallowed_extensions) + class AntiMalwareSetupTests(unittest.TestCase): """Tests setup of the `AntiMalware` cog.""" -- cgit v1.2.3 From ecaddcedab6946ac4650b699a790471ef2a898c9 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Mon, 11 May 2020 20:39:25 +0200 Subject: AntiMalware Tests - added a missing case for no extensions in test_get_disallowed_extensions --- tests/bot/cogs/test_antimalware.py | 1 + 1 file changed, 1 insertion(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 78ad996f2..7caee6f3c 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -139,6 +139,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): async def test_get_disallowed_extensions(self): """The return value should include all non-whitelisted extensions.""" test_values = ( + ([], []), (AntiMalwareConfig.whitelist, []), ([".first"], []), ([".first", ".disallowed"], [".disallowed"]), -- cgit v1.2.3 From fa467e4ef133186ff462b0178bcab08e8a3d6b2d Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Mon, 11 May 2020 20:58:51 +0200 Subject: AntiMalware Tests - Removed exact log content checks --- tests/bot/cogs/test_antimalware.py | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 7caee6f3c..a2ce9a740 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -1,5 +1,4 @@ import unittest -from os.path import splitext from unittest.mock import AsyncMock, Mock from discord import NotFound @@ -108,33 +107,17 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.message.attachments = [attachment] self.message.delete = AsyncMock(side_effect=NotFound(response=Mock(status=""), message="")) - with self.assertLogs(logger=antimalware.log, level="INFO") as logs: + with self.assertLogs(logger=antimalware.log, level="INFO"): await self.cog.on_message(self.message) self.message.delete.assert_called_once() - self.assertIn( - f"INFO:{MODULE}:Tried to delete message `{self.message.id}`, but message could not be found.", - logs.output - ) async def test_message_with_illegal_attachment_logs(self): """Deleting a message with an illegal attachment should result in a log.""" attachment = MockAttachment(filename="python.disallowed") self.message.attachments = [attachment] - file_extensions = {splitext(attachment.filename.lower())[1] for attachment in self.message.attachments} - extensions_blocked = file_extensions - set(AntiMalwareConfig.whitelist) - blocked_extensions_str = ', '.join(extensions_blocked) - - with self.assertLogs(logger=antimalware.log, level="INFO") as logs: + with self.assertLogs(logger=antimalware.log, level="INFO"): await self.cog.on_message(self.message) - self.assertEqual( - [ - f"INFO:{MODULE}:" - f"User '{self.message.author}' ({self.message.author.id}) " - f"uploaded blacklisted file(s): {blocked_extensions_str}" - ], - logs.output - ) async def test_get_disallowed_extensions(self): """The return value should include all non-whitelisted extensions.""" -- cgit v1.2.3 From d193a93828582965eb361dc6f3185291fff649a7 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:11:39 -0700 Subject: Test on_message_edit of token remover uses on_message --- tests/bot/cogs/test_token_remover.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 33d1ec170..e7b5a9bea 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,6 +1,7 @@ import asyncio import logging import unittest +from unittest import mock from unittest.mock import AsyncMock, MagicMock from discord import Colour @@ -14,7 +15,7 @@ from bot.constants import Channels, Colours, Event, Icons from tests.helpers import MockBot, MockMessage -class TokenRemoverTests(unittest.TestCase): +class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): """Tests the `TokenRemover` cog.""" def setUp(self): @@ -58,6 +59,13 @@ class TokenRemoverTests(unittest.TestCase): self.assertEqual(self.cog.mod_log, self.bot.get_cog.return_value) self.bot.get_cog.assert_called_once_with('ModLog') + async def test_on_message_edit_uses_on_message(self): + """The edit listener should delegate handling of the message to the normal listener.""" + self.cog.on_message = mock.create_autospec(self.cog.on_message, spec_set=True) + + await self.cog.on_message_edit(MockMessage(), self.msg) + self.cog.on_message.assert_awaited_once_with(self.msg) + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True @@ -77,7 +85,7 @@ class TokenRemoverTests(unittest.TestCase): for content in ('foo.bar.baz', 'x.y.'): with self.subTest(content=content): self.msg.content = content - coroutine = self.cog.on_message(self.msg) + coroutine = self.cog.is_maybe_token(self.msg) self.assertIsNone(asyncio.run(coroutine)) def test_censors_valid_tokens(self): -- cgit v1.2.3 From 0bfd003dbfc5919220129f984dc043421e535f8c Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:38:12 -0700 Subject: Add a test helper function to patch multiple attributes with autospecs This helper reduces redundancy/boilerplate by setting default values. It also has the consequence of shortening the length of the invocation, which makes it faster to use and easier to read. --- tests/helpers.py | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 2b79a6c2a..d444cc49d 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -23,6 +23,15 @@ for logger in logging.Logger.manager.loggerDict.values(): logger.setLevel(logging.CRITICAL) +def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: + """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" + # Caller's kwargs should take priority and overwrite the defaults. + kwargs = {'spec_set': True, 'autospec': True, **kwargs} + attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} + + return unittest.mock.patch.multiple(target, **attributes, **kwargs) + + class HashableMixin(discord.mixins.EqualityComparable): """ Mixin that provides similar hashing and equality functionality as discord.py's `Hashable` mixin. -- cgit v1.2.3 From e8bd69a6c556d78eca1a1eb2adfa26248273a1cd Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:42:07 -0700 Subject: Test token remover takes action if a token is found --- tests/bot/cogs/test_token_remover.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e7b5a9bea..e0ec67684 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -12,7 +12,7 @@ from bot.cogs.token_remover import ( setup as setup_cog, ) from bot.constants import Channels, Colours, Event, Icons -from tests.helpers import MockBot, MockMessage +from tests.helpers import MockBot, MockMessage, autospec class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @@ -66,6 +66,18 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): await self.cog.on_message_edit(MockMessage(), self.msg) self.cog.on_message.assert_awaited_once_with(self.msg) + @autospec(TokenRemover, "find_token_in_message", "take_action") + async def test_on_message_takes_action(self, find_token_in_message, take_action): + """Should take action if a valid token is found when a message is sent.""" + cog = TokenRemover(self.bot) + found_token = "foobar" + find_token_in_message.return_value = found_token + + await cog.on_message(self.msg) + + find_token_in_message.assert_called_once_with(self.msg) + take_action.assert_awaited_once_with(cog, self.msg, found_token) + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True -- cgit v1.2.3 From 4cf7996a1d4630ccb05f57569ca62b1798dc7a93 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:44:54 -0700 Subject: Test token remover skips messages without tokens --- tests/bot/cogs/test_token_remover.py | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e0ec67684..2b377e221 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -78,6 +78,17 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): find_token_in_message.assert_called_once_with(self.msg) take_action.assert_awaited_once_with(cog, self.msg, found_token) + @autospec(TokenRemover, "find_token_in_message", "take_action") + async def test_on_message_skips_missing_token(self, find_token_in_message, take_action): + """Shouldn't take action if a valid token isn't found when a message is sent.""" + cog = TokenRemover(self.bot) + find_token_in_message.return_value = False + + await cog.on_message(self.msg) + + find_token_in_message.assert_called_once_with(self.msg) + take_action.assert_not_awaited() + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True -- cgit v1.2.3 From 593e09299c6e4115d41bfd5b074785a5e304a8d0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:41:14 -0700 Subject: Allow using arbitrary parameter names with the autospec decorator This gives the caller more flexibility. Sometimes attribute names are too long or they don't follow a naming scheme accepted by the linter. --- tests/helpers.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index d444cc49d..1ab8b455f 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -24,12 +24,25 @@ for logger in logging.Logger.manager.loggerDict.values(): def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: - """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" + """ + Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True. + + To allow for arbitrary parameter names to be used by the decorated function, the patchers have + no attribute names associated with them. As a consequence, it will not be possible to retrieve + mocks by their attribute names when using this as a context manager, + """ # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} - return unittest.mock.patch.multiple(target, **attributes, **kwargs) + patcher = unittest.mock.patch.multiple(target, **attributes, **kwargs) + + # Unset attribute names to allow arbitrary parameter names for the decorator function. + patcher.attribute_name = None + for additional_patcher in patcher.additional_patchers: + additional_patcher.attribute_name = None + + return patcher class HashableMixin(discord.mixins.EqualityComparable): -- cgit v1.2.3 From b0dd290710799c342240d066abaebbe9e6940b54 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:09:22 -0700 Subject: Fix test for token remover ignoring bot messages It's not possible to test this via asserting the return value of `on_message` since it never returns anything. Instead, the actual relevant unit, `find_token_in_message,` should be tested. --- tests/bot/cogs/test_token_remover.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2b377e221..e8b641101 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -89,11 +89,16 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): find_token_in_message.assert_called_once_with(self.msg) take_action.assert_not_awaited() - def test_ignores_bot_messages(self): - """When the message event handler is called with a bot message, nothing is done.""" + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_ignores_bot_messages(self, token_re): + """The token finder should ignore messages authored by bots.""" + cog = TokenRemover(self.bot) self.msg.author.bot = True - coroutine = self.cog.on_message(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + + return_value = cog.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.findall.assert_not_called() def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" -- cgit v1.2.3 From 52f0f8a29d7f239c961beaa81881bf4b09da4749 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:53:06 -0700 Subject: Test `find_token_in_message` returns None if no matches found --- tests/bot/cogs/test_token_remover.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e8b641101..5932cf4f0 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -100,6 +100,20 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.findall.assert_not_called() + @autospec(TokenRemover, "is_maybe_token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): + """None should be returned if the regex matches no tokens in a message.""" + cog = TokenRemover(self.bot) + token_re.findall.return_value = () + self.msg.content = "foobar" + + return_value = cog.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.findall.assert_called_once_with(self.msg.content) + is_maybe_token.assert_not_called() + def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" for content in ('', 'lemon wins'): -- cgit v1.2.3 From cf658bd58559b2683527443f2908257f197ef0bb Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 16:06:47 -0700 Subject: Test `find_token_in_message` returns the found token --- tests/bot/cogs/test_token_remover.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5932cf4f0..2b946778b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -114,6 +114,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): token_re.findall.assert_called_once_with(self.msg.content) is_maybe_token.assert_not_called() + @autospec(TokenRemover, "is_maybe_token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_returns_found_token(self, token_re, is_maybe_token): + """The found token should be returned.""" + true_index = 1 + matches = ("foo", "bar", "baz") + side_effects = [False] * len(matches) + side_effects[true_index] = True + + cog = TokenRemover(self.bot) + self.msg.content = "foobar" + token_re.findall.return_value = matches + is_maybe_token.side_effect = side_effects + + return_value = cog.find_token_in_message(self.msg) + + self.assertEqual(return_value, matches[true_index]) + token_re.findall.assert_called_once_with(self.msg.content) + + # assert_has_calls isn't used cause it'd allow for extra calls before or after. + # The function should short-circuit, so nothing past true_index should have been used. + calls = [mock.call(match) for match in matches[:true_index + 1]] + self.assertEqual(is_maybe_token.mock_calls, calls) + def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" for content in ('', 'lemon wins'): -- cgit v1.2.3 From f92bc80d6bddb5c57c190187adaa528ae44536f6 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 16:25:14 -0700 Subject: Test token regex doesn't match invalid tokens --- tests/bot/cogs/test_token_remover.py | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2b946778b..b67602eb9 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -8,6 +8,7 @@ from discord import Colour from bot.cogs.token_remover import ( DELETION_MESSAGE_TEMPLATE, + TOKEN_RE, TokenRemover, setup as setup_cog, ) @@ -138,13 +139,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): calls = [mock.call(match) for match in matches[:true_index + 1]] self.assertEqual(is_maybe_token.mock_calls, calls) - def test_ignores_messages_without_tokens(self): - """Messages without anything looking like a token are ignored.""" - for content in ('', 'lemon wins'): - with self.subTest(content=content): - self.msg.content = content - coroutine = self.cog.on_message(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + def test_regex_invalid_tokens(self): + """Messages without anything looking like a token are not matched.""" + tokens = ( + "", + "lemon wins", + "..", + "x.y", + "x.y.", + ".y.z", + ".y.", + "..z", + "x..z", + " . . ", + "\n.\n.\n", + "'.'.'", + '"."."', + "(.(.(", + ").).)" + ) + + for token in tokens: + with self.subTest(token=token): + results = TOKEN_RE.findall(token) + self.assertEquals(len(results), 0) def test_ignores_messages_with_invalid_tokens(self): """Messages with values that are invalid tokens are ignored.""" -- cgit v1.2.3 From 34b836a8eba0f006c77a7b3f48f7ab14c37d31ee Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 17:47:09 -0700 Subject: Fix autospec decorator when used with multiple attributes The original approach of messing with the `attribute_name` didn't work for reasons I won't discuss here (would require knowledge of patcher internals). The new approach doesn't use patch.multiple but mimics it by applying multiple patch decorators to the function. As a consequence, this can no longer be used as a context manager. --- tests/helpers.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 1ab8b455f..dfbe539ec 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -24,25 +24,21 @@ for logger in logging.Logger.manager.loggerDict.values(): def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: - """ - Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True. - - To allow for arbitrary parameter names to be used by the decorated function, the patchers have - no attribute names associated with them. As a consequence, it will not be possible to retrieve - mocks by their attribute names when using this as a context manager, - """ + """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} - attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} - - patcher = unittest.mock.patch.multiple(target, **attributes, **kwargs) - - # Unset attribute names to allow arbitrary parameter names for the decorator function. - patcher.attribute_name = None - for additional_patcher in patcher.additional_patchers: - additional_patcher.attribute_name = None - return patcher + # Import the target if it's a string. + # This is to support both object and string targets like patch.multiple. + if type(target) is str: + target = unittest.mock._importer(target) + + def decorator(func): + for attribute in attributes: + patcher = unittest.mock.patch.object(target, attribute, **kwargs) + func = patcher(func) + return func + return decorator class HashableMixin(discord.mixins.EqualityComparable): -- cgit v1.2.3 From 834bd543d1d301bb853e713560a7447dc75f1ab8 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 17:53:40 -0700 Subject: Test `is_maybe_token` returns False for missing parts In practice, this won't ever happen since the regex wouldn't match strings with missing parts. However, the function does check it so may as well test it. It's not necessarily bound to always use inputs from the regex either I suppose. --- tests/bot/cogs/test_token_remover.py | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index b67602eb9..9e1d96a37 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -164,6 +164,16 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = TOKEN_RE.findall(token) self.assertEquals(len(results), 0) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): + """False should be returned for tokens which do not have all 3 parts.""" + cog = TokenRemover(self.bot) + return_value = cog.is_maybe_token("x.y") + + self.assertFalse(return_value) + valid_user.assert_not_called() + valid_time.assert_not_called() + def test_ignores_messages_with_invalid_tokens(self): """Messages with values that are invalid tokens are ignored.""" for content in ('foo.bar.baz', 'x.y.'): -- cgit v1.2.3 From ab5d194b90a7e068c8ab7171939f471e252ee073 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:11:31 -0700 Subject: Test is_maybe_token --- tests/bot/cogs/test_token_remover.py | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 9e1d96a37..85bbbdf6b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -174,13 +174,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): valid_user.assert_not_called() valid_time.assert_not_called() - def test_ignores_messages_with_invalid_tokens(self): - """Messages with values that are invalid tokens are ignored.""" - for content in ('foo.bar.baz', 'x.y.'): - with self.subTest(content=content): - self.msg.content = content - coroutine = self.cog.is_maybe_token(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + def test_is_maybe_token(self, valid_user, valid_time): + """Should return True if the user ID and timestamp are valid or return False otherwise.""" + cog = TokenRemover(self.bot) + subtests = ( + (False, True, False), + (True, False, False), + (True, True, True), + ) + + for user_return, time_return, expected in subtests: + valid_user.reset_mock() + valid_time.reset_mock() + + with self.subTest(user_return=user_return, time_return=time_return, expected=expected): + valid_user.return_value = user_return + valid_time.return_value = time_return + + actual = cog.is_maybe_token("x.y.z") + self.assertIs(actual, expected) + + valid_user.assert_called_once_with("x") + if user_return: + valid_time.assert_called_once_with("y") def test_censors_valid_tokens(self): """Valid tokens are censored.""" -- cgit v1.2.3 From 4b6fde69a7e193382701dccf80a5471ea7ccea72 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:22:31 -0700 Subject: Test token regex matches valid tokens --- tests/bot/cogs/test_token_remover.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 85bbbdf6b..7310b4637 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -164,6 +164,27 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = TOKEN_RE.findall(token) self.assertEquals(len(results), 0) + def test_regex_valid_tokens(self): + """Messages that look like tokens should be matched.""" + # Don't worry, the token's been invalidated. + tokens = ( + "x1.y2.z_3", + "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8" + ) + + for token in tokens: + with self.subTest(token=token): + results = TOKEN_RE.findall(token) + self.assertIn(token, results) + + def test_regex_matches_multiple_valid(self): + """Should support multiple matches in the middle of a string.""" + tokens = ["x.y.z", "a.b.c"] + message = f"garbage {tokens[0]} hello {tokens[1]} world" + + results = TOKEN_RE.findall(message) + self.assertEquals(tokens, results) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): """False should be returned for tokens which do not have all 3 parts.""" -- cgit v1.2.3 From d8d8e144adfe4c2de15dbbf4346e2eec548a9f67 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:28:06 -0700 Subject: Correct the return type annotation for the autospec decorator --- tests/helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index dfbe539ec..3cd8a63c0 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -4,7 +4,7 @@ import collections import itertools import logging import unittest.mock -from typing import Iterable, Optional +from typing import Callable, Iterable, Optional import discord from discord.ext.commands import Context @@ -23,7 +23,7 @@ for logger in logging.Logger.manager.loggerDict.values(): logger.setLevel(logging.CRITICAL) -def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: +def autospec(target, *attributes: str, **kwargs) -> Callable: """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} -- cgit v1.2.3 From 5b9bf9aba686f570322cb9996dd35d3ab669a162 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:26:16 -0700 Subject: Avoid instantiating the cog when testing static/class methods --- tests/bot/cogs/test_token_remover.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 7310b4637..6a8247070 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -93,10 +93,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_ignores_bot_messages(self, token_re): """The token finder should ignore messages authored by bots.""" - cog = TokenRemover(self.bot) self.msg.author.bot = True - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) token_re.findall.assert_not_called() @@ -105,11 +104,10 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" - cog = TokenRemover(self.bot) token_re.findall.return_value = () self.msg.content = "foobar" - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) token_re.findall.assert_called_once_with(self.msg.content) @@ -124,12 +122,11 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): side_effects = [False] * len(matches) side_effects[true_index] = True - cog = TokenRemover(self.bot) self.msg.content = "foobar" token_re.findall.return_value = matches is_maybe_token.side_effect = side_effects - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertEqual(return_value, matches[true_index]) token_re.findall.assert_called_once_with(self.msg.content) @@ -188,8 +185,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): """False should be returned for tokens which do not have all 3 parts.""" - cog = TokenRemover(self.bot) - return_value = cog.is_maybe_token("x.y") + return_value = TokenRemover.is_maybe_token("x.y") self.assertFalse(return_value) valid_user.assert_not_called() @@ -198,7 +194,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token(self, valid_user, valid_time): """Should return True if the user ID and timestamp are valid or return False otherwise.""" - cog = TokenRemover(self.bot) subtests = ( (False, True, False), (True, False, False), @@ -213,7 +208,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): valid_user.return_value = user_return valid_time.return_value = time_return - actual = cog.is_maybe_token("x.y.z") + actual = TokenRemover.is_maybe_token("x.y.z") self.assertIs(actual, expected) valid_user.assert_called_once_with("x") -- cgit v1.2.3 From 2127239840085ba523d411899e0b7a188530df07 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:33:05 -0700 Subject: Simplify token remover's message mock * Rely on default values for the author * Set the content to a non-empty string --- tests/bot/cogs/test_token_remover.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 6a8247070..5ca863926 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -26,14 +26,10 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.bot.get_cog.return_value.send_log_message = AsyncMock() self.cog = TokenRemover(bot=self.bot) - self.msg = MockMessage(id=555, content='') - self.msg.author.__str__ = MagicMock() - self.msg.author.__str__.return_value = 'lemon' - self.msg.author.bot = False - self.msg.author.avatar_url_as.return_value = 'picture-lemon.png' - self.msg.author.id = 42 - self.msg.author.mention = '@lemon' + self.msg = MockMessage(id=555, content="hello world") self.msg.channel.mention = "#lemonade-stand" + self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) + self.msg.author.avatar_url_as.return_value = "picture-lemon.png" def test_is_valid_user_id_is_true_for_numeric_content(self): """A string decoding to numeric characters is a valid user ID.""" @@ -105,7 +101,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" token_re.findall.return_value = () - self.msg.content = "foobar" return_value = TokenRemover.find_token_in_message(self.msg) @@ -122,7 +117,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): side_effects = [False] * len(matches) side_effects[true_index] = True - self.msg.content = "foobar" token_re.findall.return_value = matches is_maybe_token.side_effect = side_effects -- cgit v1.2.3 From e4790b330da1605573b5d23615bfe62b481e1e04 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:37:59 -0700 Subject: Test token remover's message deletion --- tests/bot/cogs/test_token_remover.py | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5ca863926..d65ce2ce5 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -209,6 +209,15 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): if user_return: valid_time.assert_called_once_with("y") + async def test_delete_message(self): + """The message should be deleted, and a message should be sent to the same channel.""" + await TokenRemover.delete_message(self.msg) + + self.msg.delete.assert_called_once_with() + self.msg.channel.send.assert_called_once_with( + DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + ) + def test_censors_valid_tokens(self): """Valid tokens are censored.""" cases = ( -- cgit v1.2.3 From 567a5f9242912d6a3340c088c0ae1a62977a141e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:46:02 -0700 Subject: Test TokenRemover.format_log_message --- tests/bot/cogs/test_token_remover.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index d65ce2ce5..f5412e692 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -218,6 +218,22 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) ) + @autospec("bot.cogs.token_remover", "LOG_MESSAGE") + async def test_format_log_message(self, log_message): + """Should correctly format the log message with info from the message and token.""" + log_message.format.return_value = "Howdy" + return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") + + self.assertEqual(return_value, log_message.format.return_value) + log_message.format.assert_called_once_with( + author=self.msg.author, + author_id=self.msg.author.id, + channel=self.msg.channel.mention, + user_id="MTIz", + timestamp="DN9R_A", + hmac="xxx", + ) + def test_censors_valid_tokens(self): """Valid tokens are censored.""" cases = ( -- cgit v1.2.3 From f47cbef0b47ef11b8c1fd63076105e4cb7d73601 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:29:28 -0700 Subject: Test TokenRemover.take_action * Remove `bot.get_cog` mocks in `setUp` * Mock the logger cause it's easier to assert logs * Remove subtests * Assert helper functions were called * Create an autospec for ModLog --- tests/bot/cogs/test_token_remover.py | 73 +++++++++++++++--------------------- 1 file changed, 30 insertions(+), 43 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index f5412e692..3546e7964 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,11 +1,10 @@ -import asyncio -import logging import unittest from unittest import mock -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import MagicMock from discord import Colour +from bot.cogs.moderation import ModLog from bot.cogs.token_remover import ( DELETION_MESSAGE_TEMPLATE, TOKEN_RE, @@ -22,8 +21,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def setUp(self): """Adds the cog, a bot, and a message to the instance for usage in tests.""" self.bot = MockBot() - self.bot.get_cog.return_value = MagicMock() - self.bot.get_cog.return_value.send_log_message = AsyncMock() self.cog = TokenRemover(bot=self.bot) self.msg = MockMessage(id=555, content="hello world") @@ -234,46 +231,36 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): hmac="xxx", ) - def test_censors_valid_tokens(self): - """Valid tokens are censored.""" - cases = ( - # (content, censored_token) - ('MTIz.DN9R_A.xyz', 'MTIz.DN9R_A.xxx'), + @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) + @autospec("bot.cogs.token_remover", "log") + @autospec(TokenRemover, "delete_message", "format_log_message") + async def test_take_action(self, delete_message, format_log_message, logger, mod_log_property): + """Should delete the message and send a mod log.""" + cog = TokenRemover(self.bot) + mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) + token = "MTIz.DN9R_A.xyz" + log_msg = "testing123" + + mod_log_property.return_value = mod_log + format_log_message.return_value = log_msg + + await cog.take_action(self.msg, token) + + delete_message.assert_awaited_once_with(self.msg) + format_log_message.assert_called_once_with(self.msg, token) + logger.debug.assert_called_with(log_msg) + self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") + + mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) + mod_log.send_log_message.assert_called_once_with( + icon_url=Icons.token_removed, + colour=Colour(Colours.soft_red), + title="Token removed!", + text=log_msg, + thumbnail=self.msg.author.avatar_url_as.return_value, + channel_id=Channels.mod_alerts ) - for content, censored_token in cases: - with self.subTest(content=content, censored_token=censored_token): - self.msg.content = content - coroutine = self.cog.on_message(self.msg) - with self.assertLogs(logger='bot.cogs.token_remover', level=logging.DEBUG) as cm: - self.assertIsNone(asyncio.run(coroutine)) # no return value - - [line] = cm.output - log_message = ( - "Censored a seemingly valid token sent by " - "lemon (`42`) in #lemonade-stand, " - f"token was `{censored_token}`" - ) - self.assertIn(log_message, line) - - self.msg.delete.assert_called_once_with() - self.msg.channel.send.assert_called_once_with( - DELETION_MESSAGE_TEMPLATE.format(mention='@lemon') - ) - self.bot.get_cog.assert_called_with('ModLog') - self.msg.author.avatar_url_as.assert_called_once_with(static_format='png') - - mod_log = self.bot.get_cog.return_value - mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) - mod_log.send_log_message.assert_called_once_with( - icon_url=Icons.token_removed, - colour=Colour(Colours.soft_red), - title="Token removed!", - text=log_message, - thumbnail='picture-lemon.png', - channel_id=Channels.mod_alerts - ) - class TokenRemoverSetupTests(unittest.TestCase): """Tests setup of the `TokenRemover` cog.""" -- cgit v1.2.3 From 5734a4d84922a9497014dfeb3eba31ad3c57536f Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:44:08 -0700 Subject: Refactor `TokenRemoverSetupTests` and add a more thorough test The test now ensures the cog is instantiated and that the instance is passed as an argument to `add_cog`. --- tests/bot/cogs/test_token_remover.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 3546e7964..c377de7b2 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -262,11 +262,15 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): ) -class TokenRemoverSetupTests(unittest.TestCase): - """Tests setup of the `TokenRemover` cog.""" +class TokenRemoverExtensionTests(unittest.TestCase): + """Tests for the token_remover extension.""" - def test_setup(self): - """Setup of the extension should call add_cog.""" + @autospec("bot.cogs.token_remover", "TokenRemover") + def test_extension_setup(self, cog): + """The TokenRemover cog should be added.""" bot = MockBot() setup_cog(bot) + + cog.assert_called_once_with(bot) bot.add_cog.assert_called_once() + self.assertTrue(isinstance(bot.add_cog.call_args.args[0], TokenRemover)) -- cgit v1.2.3 From d0303d715d485842a2d5c906099d767d74cf8bd8 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:45:50 -0700 Subject: Replace deprecated assertion methods --- tests/bot/cogs/test_token_remover.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index c377de7b2..aecb51403 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -150,7 +150,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): results = TOKEN_RE.findall(token) - self.assertEquals(len(results), 0) + self.assertEqual(len(results), 0) def test_regex_valid_tokens(self): """Messages that look like tokens should be matched.""" @@ -171,7 +171,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): message = f"garbage {tokens[0]} hello {tokens[1]} world" results = TOKEN_RE.findall(message) - self.assertEquals(tokens, results) + self.assertEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- cgit v1.2.3 From 862153f2e4ab5b1408719fb2c1abc5143cfb15ce Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:47:40 -0700 Subject: Clean up token remover test imports --- tests/bot/cogs/test_token_remover.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index aecb51403..5cc8c7ad1 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -4,14 +4,10 @@ from unittest.mock import MagicMock from discord import Colour +from bot import constants +from bot.cogs import token_remover from bot.cogs.moderation import ModLog -from bot.cogs.token_remover import ( - DELETION_MESSAGE_TEMPLATE, - TOKEN_RE, - TokenRemover, - setup as setup_cog, -) -from bot.constants import Channels, Colours, Event, Icons +from bot.cogs.token_remover import TokenRemover from tests.helpers import MockBot, MockMessage, autospec @@ -149,7 +145,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = TOKEN_RE.findall(token) + results = token_remover.TOKEN_RE.findall(token) self.assertEqual(len(results), 0) def test_regex_valid_tokens(self): @@ -162,7 +158,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = TOKEN_RE.findall(token) + results = token_remover.TOKEN_RE.findall(token) self.assertIn(token, results) def test_regex_matches_multiple_valid(self): @@ -170,7 +166,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): tokens = ["x.y.z", "a.b.c"] message = f"garbage {tokens[0]} hello {tokens[1]} world" - results = TOKEN_RE.findall(message) + results = token_remover.TOKEN_RE.findall(message) self.assertEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") @@ -212,7 +208,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.delete.assert_called_once_with() self.msg.channel.send.assert_called_once_with( - DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) ) @autospec("bot.cogs.token_remover", "LOG_MESSAGE") @@ -251,14 +247,14 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): logger.debug.assert_called_with(log_msg) self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") - mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) + mod_log.ignore.assert_called_once_with(constants.Event.message_delete, self.msg.id) mod_log.send_log_message.assert_called_once_with( - icon_url=Icons.token_removed, - colour=Colour(Colours.soft_red), + icon_url=constants.Icons.token_removed, + colour=Colour(constants.Colours.soft_red), title="Token removed!", text=log_msg, thumbnail=self.msg.author.avatar_url_as.return_value, - channel_id=Channels.mod_alerts + channel_id=constants.Channels.mod_alerts ) @@ -269,7 +265,7 @@ class TokenRemoverExtensionTests(unittest.TestCase): def test_extension_setup(self, cog): """The TokenRemover cog should be added.""" bot = MockBot() - setup_cog(bot) + token_remover.setup(bot) cog.assert_called_once_with(bot) bot.add_cog.assert_called_once() -- cgit v1.2.3 From 4701b0da36c7f42792c0af258b785076237fd661 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:56:15 -0700 Subject: Use subtests for valid ID/timestamp tests and test non-ASCII inputs --- tests/bot/cogs/test_token_remover.py | 43 +++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 18 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5cc8c7ad1..f1a56c235 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -24,24 +24,31 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) self.msg.author.avatar_url_as.return_value = "picture-lemon.png" - def test_is_valid_user_id_is_true_for_numeric_content(self): - """A string decoding to numeric characters is a valid user ID.""" - # MTIz = base64(123) - self.assertTrue(TokenRemover.is_valid_user_id('MTIz')) - - def test_is_valid_user_id_is_false_for_alphabetic_content(self): - """A string decoding to alphabetic characters is not a valid user ID.""" - # YWJj = base64(abc) - self.assertFalse(TokenRemover.is_valid_user_id('YWJj')) - - def test_is_valid_timestamp_is_true_for_valid_timestamps(self): - """A string decoding to a valid timestamp should be recognized as such.""" - self.assertTrue(TokenRemover.is_valid_timestamp('DN9r_A')) - - def test_is_valid_timestamp_is_false_for_invalid_values(self): - """A string not decoding to a valid timestamp should not be recognized as such.""" - # MTIz = base64(123) - self.assertFalse(TokenRemover.is_valid_timestamp('MTIz')) + def test_is_valid_user_id(self): + """Should correctly discern valid user IDs and ignore non-numeric and non-ASCII IDs.""" + subtests = ( + ("MTIz", True), # base64(123) + ("YWJj", False), # base64(abc) + ("λδµ", False), + ) + + for user_id, is_valid in subtests: + with self.subTest(user_id=user_id, is_valid=is_valid): + result = TokenRemover.is_valid_user_id(user_id) + self.assertIs(result, is_valid) + + def test_is_valid_timestamp(self): + """Should correctly discern valid timestamps.""" + subtests = ( + ("DN9r_A", True), + ("MTIz", False), # base64(123) + ("λδµ", False), + ) + + for timestamp, is_valid in subtests: + with self.subTest(timestamp=timestamp, is_valid=is_valid): + result = TokenRemover.is_valid_timestamp(timestamp) + self.assertIs(result, is_valid) def test_mod_log_property(self): """The `mod_log` property should ask the bot to return the `ModLog` cog.""" -- cgit v1.2.3 From ddfe583d0b1e72f98855f628ff01b72c82fa491d Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Mon, 11 May 2020 21:56:39 +0200 Subject: AntiMalware Refactor - Moved embed descriptions into constants, added tests for embed descriptions --- bot/cogs/antimalware.py | 44 ++++++++++++++++++++-------------- tests/bot/cogs/test_antimalware.py | 48 ++++++++++++++++++++++++-------------- 2 files changed, 56 insertions(+), 36 deletions(-) (limited to 'tests') diff --git a/bot/cogs/antimalware.py b/bot/cogs/antimalware.py index f5fd5e2d9..ea257442e 100644 --- a/bot/cogs/antimalware.py +++ b/bot/cogs/antimalware.py @@ -10,6 +10,27 @@ from bot.constants import AntiMalware as AntiMalwareConfig, Channels, STAFF_ROLE log = logging.getLogger(__name__) +PY_EMBED_DESCRIPTION = ( + "It looks like you tried to attach a Python file - " + f"please use a code-pasting service such as {URLs.site_schema}{URLs.site_paste}" +) + +TXT_EMBED_DESCRIPTION = ( + "**Uh-oh!** It looks like your message got zapped by our spam filter. " + "We currently don't allow `.txt` attachments, so here are some tips to help you travel safely: \n\n" + "• If you attempted to send a message longer than 2000 characters, try shortening your message " + "to fit within the character limit or use a pasting service (see below) \n\n" + "• If you tried to show someone your code, you can use codeblocks \n(run `!code-blocks` in " + "{cmd_channel_mention} for more information) or use a pasting service like: " + f"\n\n{URLs.site_schema}{URLs.site_paste}" +) + +DISALLOWED_EMBED_DESCRIPTION = ( + "It looks like you tried to attach file type(s) that we do not allow ({blocked_extensions_str}). " + f"We currently allow the following file types: **{', '.join(AntiMalwareConfig.whitelist)}**.\n\n" + "Feel free to ask in {meta_channel_mention} if you think this is a mistake." +) + class AntiMalware(Cog): """Delete messages which contain attachments with non-whitelisted file extensions.""" @@ -34,29 +55,16 @@ class AntiMalware(Cog): blocked_extensions_str = ', '.join(extensions_blocked) if ".py" in extensions_blocked: # Short-circuit on *.py files to provide a pastebin link - embed.description = ( - "It looks like you tried to attach a Python file - " - f"please use a code-pasting service such as {URLs.site_schema}{URLs.site_paste}" - ) + embed.description = PY_EMBED_DESCRIPTION elif ".txt" in extensions_blocked: # Work around Discord AutoConversion of messages longer than 2000 chars to .txt cmd_channel = self.bot.get_channel(Channels.bot_commands) - embed.description = ( - "**Uh-oh!** It looks like your message got zapped by our spam filter. " - "We currently don't allow `.txt` attachments, so here are some tips to help you travel safely: \n\n" - "• If you attempted to send a message longer than 2000 characters, try shortening your message " - "to fit within the character limit or use a pasting service (see below) \n\n" - "• If you tried to show someone your code, you can use codeblocks \n(run `!code-blocks` in " - f"{cmd_channel.mention} for more information) or use a pasting service like: " - f"\n\n{URLs.site_schema}{URLs.site_paste}" - ) + embed.description = TXT_EMBED_DESCRIPTION.format(cmd_channel_mention=cmd_channel.mention) elif extensions_blocked: - whitelisted_types = ', '.join(AntiMalwareConfig.whitelist) meta_channel = self.bot.get_channel(Channels.meta) - embed.description = ( - f"It looks like you tried to attach file type(s) that we do not allow ({blocked_extensions_str}). " - f"We currently allow the following file types: **{whitelisted_types}**.\n\n" - f"Feel free to ask in {meta_channel.mention} if you think this is a mistake." + embed.description = DISALLOWED_EMBED_DESCRIPTION.format( + blocked_extensions_str=blocked_extensions_str, + meta_channel_mention=meta_channel.mention, ) if embed.description: diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index a2ce9a740..fab063201 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -4,7 +4,7 @@ from unittest.mock import AsyncMock, Mock from discord import NotFound from bot.cogs import antimalware -from bot.constants import AntiMalware as AntiMalwareConfig, Channels, STAFF_ROLES, URLs +from bot.constants import AntiMalware as AntiMalwareConfig, Channels, STAFF_ROLES from tests.helpers import MockAttachment, MockBot, MockMessage, MockRole MODULE = "bot.cogs.antimalware" @@ -63,7 +63,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.message.delete.assert_not_called() - async def test_python_file_redirect_embed(self): + async def test_python_file_redirect_embed_description(self): """A message containing a .py file should result in an embed redirecting the user to our paste site""" attachment = MockAttachment(filename="python.py") self.message.attachments = [attachment] @@ -74,32 +74,44 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): args, kwargs = self.message.channel.send.call_args embed = kwargs.pop("embed") - self.assertEqual(args[0], f"Hey {self.message.author.mention}!") - self.assertEqual(embed.description, ( - "It looks like you tried to attach a Python file - " - f"please use a code-pasting service such as {URLs.site_schema}{URLs.site_paste}" - )) + self.assertEqual(embed.description, antimalware.PY_EMBED_DESCRIPTION) - async def test_txt_file_redirect_embed(self): + async def test_txt_file_redirect_embed_description(self): + """A message containing a .txt file should result in the correct embed.""" attachment = MockAttachment(filename="python.txt") self.message.attachments = [attachment] self.message.channel.send = AsyncMock() + antimalware.TXT_EMBED_DESCRIPTION = Mock() + antimalware.TXT_EMBED_DESCRIPTION.format.return_value = "test" await self.cog.on_message(self.message) + self.message.channel.send.assert_called_once() args, kwargs = self.message.channel.send.call_args embed = kwargs.pop("embed") cmd_channel = self.bot.get_channel(Channels.bot_commands) - self.assertEqual(args[0], f"Hey {self.message.author.mention}!") - self.assertEqual(embed.description, ( - "**Uh-oh!** It looks like your message got zapped by our spam filter. " - "We currently don't allow `.txt` attachments, so here are some tips to help you travel safely: \n\n" - "• If you attempted to send a message longer than 2000 characters, try shortening your message " - "to fit within the character limit or use a pasting service (see below) \n\n" - "• If you tried to show someone your code, you can use codeblocks \n(run `!code-blocks` in " - f"{cmd_channel.mention} for more information) or use a pasting service like: " - f"\n\n{URLs.site_schema}{URLs.site_paste}" - )) + self.assertEqual(embed.description, antimalware.TXT_EMBED_DESCRIPTION.format.return_value) + antimalware.TXT_EMBED_DESCRIPTION.format.assert_called_with(cmd_channel_mention=cmd_channel.mention) + + async def test_other_disallowed_extention_embed_description(self): + """Test the description for a non .py/.txt disallowed extension.""" + attachment = MockAttachment(filename="python.disallowed") + self.message.attachments = [attachment] + self.message.channel.send = AsyncMock() + antimalware.DISALLOWED_EMBED_DESCRIPTION = Mock() + antimalware.DISALLOWED_EMBED_DESCRIPTION.format.return_value = "test" + + await self.cog.on_message(self.message) + self.message.channel.send.assert_called_once() + args, kwargs = self.message.channel.send.call_args + embed = kwargs.pop("embed") + meta_channel = self.bot.get_channel(Channels.meta) + + self.assertEqual(embed.description, antimalware.DISALLOWED_EMBED_DESCRIPTION.format.return_value) + antimalware.DISALLOWED_EMBED_DESCRIPTION.format.assert_called_with( + blocked_extensions_str=".disallowed", + meta_channel_mention=meta_channel.mention + ) async def test_removing_deleted_message_logs(self): """Removing an already deleted message logs the correct message""" -- cgit v1.2.3 From 31aff51655d3783bc70f04628f189cf3c3591028 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 13 May 2020 18:58:43 -0700 Subject: Fix a test needlessly being a coroutine --- tests/bot/cogs/test_token_remover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index f1a56c235..8e743a715 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -219,7 +219,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): ) @autospec("bot.cogs.token_remover", "LOG_MESSAGE") - async def test_format_log_message(self, log_message): + def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" log_message.format.return_value = "Howdy" return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") -- cgit v1.2.3 From 5a48ed0d60ebc9984cae27b19953b50b52df83d9 Mon Sep 17 00:00:00 2001 From: Numerlor <25886452+Numerlor@users.noreply.github.com> Date: Fri, 15 May 2020 19:52:26 +0200 Subject: Change tests to use the new timeout constant --- tests/bot/cogs/test_snekbox.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_snekbox.py b/tests/bot/cogs/test_snekbox.py index 1dec0ccaf..ccc090f02 100644 --- a/tests/bot/cogs/test_snekbox.py +++ b/tests/bot/cogs/test_snekbox.py @@ -291,7 +291,11 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(actual, expected) self.bot.wait_for.assert_has_awaits( ( - call('message_edit', check=partial_mock(snekbox.predicate_eval_message_edit, ctx), timeout=10), + call( + 'message_edit', + check=partial_mock(snekbox.predicate_eval_message_edit, ctx), + timeout=snekbox.REEVAL_TIMEOUT, + ), call('reaction_add', check=partial_mock(snekbox.predicate_eval_emoji_reaction, ctx), timeout=10) ) ) -- cgit v1.2.3 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 21916ad9c19a326eb8406ea751e5fd9f80e9d912 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Wed, 20 May 2020 10:42:11 +0300 Subject: ModLog Tests: Fix truncation tests docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Leon Sandøy --- tests/bot/cogs/moderation/test_modlog.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_modlog.py b/tests/bot/cogs/moderation/test_modlog.py index d60836474..b5ad21a09 100644 --- a/tests/bot/cogs/moderation/test_modlog.py +++ b/tests/bot/cogs/moderation/test_modlog.py @@ -15,7 +15,7 @@ class ModLogTests(unittest.IsolatedAsyncioTestCase): self.channel = MockTextChannel() async def test_log_entry_description_truncation(self): - """Should truncate embed description for ModLog entry.""" + """Test that embed description for ModLog entry is truncated.""" self.bot.get_channel.return_value = self.channel await self.cog.send_log_message( icon_url="foo", -- cgit v1.2.3 From 1432e5ba36fc09c7233e5be4745f540c2c4af792 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Wed, 20 May 2020 10:47:59 +0300 Subject: Infraction Tests: Small fixes - Remove unnecessary space from placeholder - Rename `has_active_infraction` to `get_active_infraction` --- tests/bot/cogs/moderation/test_infractions.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py index 51a8cc645..2b1ff5728 100644 --- a/tests/bot/cogs/moderation/test_infractions.py +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -17,11 +17,11 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): self.guild = MockGuild(id=4567) self.ctx = MockContext(bot=self.bot, author=self.user, guild=self.guild) - @patch("bot.cogs.moderation.utils.has_active_infraction") + @patch("bot.cogs.moderation.utils.get_active_infraction") @patch("bot.cogs.moderation.utils.post_infraction") - async def test_apply_ban_reason_truncation(self, post_infraction_mock, has_active_mock): + async def test_apply_ban_reason_truncation(self, post_infraction_mock, get_active_mock): """Should truncate reason for `ctx.guild.ban`.""" - has_active_mock.return_value = False + get_active_mock.return_value = 'foo' post_infraction_mock.return_value = {"foo": "bar"} self.cog.apply_infraction = AsyncMock() @@ -32,7 +32,7 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): ban = self.cog.apply_infraction.call_args[0][3] self.assertEqual( ban.cr_frame.f_locals["kwargs"]["reason"], - textwrap.shorten("foo bar" * 3000, 512, placeholder=" ...") + textwrap.shorten("foo bar" * 3000, 512, placeholder="...") ) # Await ban to avoid warning await ban -- cgit v1.2.3 From 5989bcfefa244eb05f37b76d1e1df2f45e5782fa Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Wed, 20 May 2020 10:54:49 +0300 Subject: ModLog Tests: Fix embed description truncate test --- tests/bot/cogs/moderation/test_modlog.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_modlog.py b/tests/bot/cogs/moderation/test_modlog.py index b5ad21a09..f2809f40a 100644 --- a/tests/bot/cogs/moderation/test_modlog.py +++ b/tests/bot/cogs/moderation/test_modlog.py @@ -25,5 +25,5 @@ class ModLogTests(unittest.IsolatedAsyncioTestCase): ) embed = self.channel.send.call_args[1]["embed"] self.assertEqual( - embed.description, ("foo bar" * 3000)[:2046] + "..." + embed.description, ("foo bar" * 3000)[:2045] + "..." ) -- cgit v1.2.3 From 874cb001df91ea8223385dd2b32ab4e3c280e183 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Wed, 20 May 2020 10:57:07 +0300 Subject: Infr. Tests: Add more content to await comment --- tests/bot/cogs/moderation/test_infractions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py index 2b1ff5728..f8f340c2e 100644 --- a/tests/bot/cogs/moderation/test_infractions.py +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -34,7 +34,7 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): ban.cr_frame.f_locals["kwargs"]["reason"], textwrap.shorten("foo bar" * 3000, 512, placeholder="...") ) - # Await ban to avoid warning + # Await ban to avoid not awaited coroutine warning await ban @patch("bot.cogs.moderation.utils.post_infraction") @@ -51,5 +51,5 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): kick.cr_frame.f_locals["kwargs"]["reason"], textwrap.shorten("foo bar" * 3000, 512, placeholder="...") ) - # Await kick to avoid warning + # Await kick to avoid not awaited coroutine warning await kick -- cgit v1.2.3 From e9bd09d90c5acf61caa955533f406851e1a65aec Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Wed, 20 May 2020 10:59:11 +0300 Subject: Infr. Tests: Replace `str` with `dict` To allow `.get`, I had to replace `str` return value with `dict` --- tests/bot/cogs/moderation/test_infractions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py index f8f340c2e..139209749 100644 --- a/tests/bot/cogs/moderation/test_infractions.py +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -21,7 +21,7 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): @patch("bot.cogs.moderation.utils.post_infraction") async def test_apply_ban_reason_truncation(self, post_infraction_mock, get_active_mock): """Should truncate reason for `ctx.guild.ban`.""" - get_active_mock.return_value = 'foo' + get_active_mock.return_value = {"foo": "bar"} post_infraction_mock.return_value = {"foo": "bar"} self.cog.apply_infraction = AsyncMock() -- cgit v1.2.3 From d9730e41b3144862fdd9c221d160a40144a7c881 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Wed, 20 May 2020 11:02:37 +0300 Subject: Infr. Test: Replace `get_active_mock` return value Replace `{"foo": "bar"}` with `{"id": 1}` --- tests/bot/cogs/moderation/test_infractions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py index 139209749..925439bf3 100644 --- a/tests/bot/cogs/moderation/test_infractions.py +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -21,7 +21,7 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): @patch("bot.cogs.moderation.utils.post_infraction") async def test_apply_ban_reason_truncation(self, post_infraction_mock, get_active_mock): """Should truncate reason for `ctx.guild.ban`.""" - get_active_mock.return_value = {"foo": "bar"} + get_active_mock.return_value = {"id": 1} post_infraction_mock.return_value = {"foo": "bar"} self.cog.apply_infraction = AsyncMock() -- cgit v1.2.3 From a1b6d147befd4043acdddc00667d3bda94cc76ad Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Wed, 20 May 2020 11:15:09 +0300 Subject: Infr Tests: Make `get_active_infraction` return `None` --- tests/bot/cogs/moderation/test_infractions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py index 925439bf3..5548d9f68 100644 --- a/tests/bot/cogs/moderation/test_infractions.py +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -21,7 +21,7 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): @patch("bot.cogs.moderation.utils.post_infraction") async def test_apply_ban_reason_truncation(self, post_infraction_mock, get_active_mock): """Should truncate reason for `ctx.guild.ban`.""" - get_active_mock.return_value = {"id": 1} + get_active_mock.return_value = None post_infraction_mock.return_value = {"foo": "bar"} self.cog.apply_infraction = AsyncMock() -- 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 0ede719d7beb36f476ac26f948ab940882978476 Mon Sep 17 00:00:00 2001 From: Jannes Jonkers Date: Mon, 25 May 2020 20:44:35 +0200 Subject: AntiMalware tests - Switched from monkeypatch to unittest.patch --- tests/bot/cogs/test_antimalware.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index fab063201..f219fc1ba 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -1,5 +1,5 @@ import unittest -from unittest.mock import AsyncMock, Mock +from unittest.mock import AsyncMock, Mock, patch from discord import NotFound @@ -10,6 +10,7 @@ from tests.helpers import MockAttachment, MockBot, MockMessage, MockRole MODULE = "bot.cogs.antimalware" +@patch(f"{MODULE}.AntiMalwareConfig.whitelist", new=[".first", ".second", ".third"]) class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): """Test the AntiMalware cog.""" @@ -18,7 +19,6 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.bot = MockBot() self.cog = antimalware.AntiMalware(self.bot) self.message = MockMessage() - AntiMalwareConfig.whitelist = [".first", ".second", ".third"] async def test_message_with_allowed_attachment(self): """Messages with allowed extensions should not be deleted""" -- 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 47886501fb7d030f1cb91c69413058e3ffcb76bf Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 20:47:32 -0700 Subject: Test token regex won't match non-base64 characters --- tests/bot/cogs/test_token_remover.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 8e743a715..dbea5ad1b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -144,10 +144,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): "x..z", " . . ", "\n.\n.\n", - "'.'.'", - '"."."', - "(.(.(", - ").).)" + "hellö.world.bye", + "base64.nötbåse64.morebase64", + "19jd3J.dfkm3d.€víł§tüff", ) for token in tokens: -- cgit v1.2.3 From e76099d48b9a895c48e58c5f5489886f4191eeb6 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 20:50:30 -0700 Subject: Add more valid tokens to test the regex with --- tests/bot/cogs/test_token_remover.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index dbea5ad1b..6a280f358 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -156,10 +156,12 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_regex_valid_tokens(self): """Messages that look like tokens should be matched.""" - # Don't worry, the token's been invalidated. + # Don't worry, these tokens have been invalidated. tokens = ( - "x1.y2.z_3", - "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8" + "NDcyMjY1OTQzMDYy_DEzMz-y.XsyRkw.VXmErH7j511turNpfURmb0rVNm8", + "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8", + "NDc1MDczNjI5Mzk5NTQ3OTA0.XsyR-w.sJf6omBPORBPju3WJEIAcwW9Zds", + "NDY3MjIzMjMwNjUwNzc3NjQx.XsySD_.s45jqDV_Iisn-symw0yDRrk_jf4", ) for token in tokens: -- cgit v1.2.3 From a8a216d0803b67a330ae092a17bea563f5012275 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:02:24 -0700 Subject: Fix valid token regex test It was broken due to the addition of groups. Rather than returning the full match, `findall` returns groups if any exist. The test was comparing a tuple of groups to the token string, which was of course failing. Now `fullmatch` is used cause it's simpler - just check for `None` and don't worry about iterating matches to search. --- tests/bot/cogs/test_token_remover.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 6a280f358..518bf91ca 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -166,8 +166,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = token_remover.TOKEN_RE.findall(token) - self.assertIn(token, results) + results = token_remover.TOKEN_RE.fullmatch(token) + self.assertIsNotNone(results, f"{token} was not matched by the regex") def test_regex_matches_multiple_valid(self): """Should support multiple matches in the middle of a string.""" -- cgit v1.2.3 From 19cc849d4c70bc3e792460ad712aa308fa500462 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:07:21 -0700 Subject: Fix multiple match text for token regex It has to account for the addition of groups. It's easiest to compare the entire string so `finditer` is used to return re.Match objects; the tuples of `findall` would be cumbersome. Also threw in a change to use `assertCountEqual` cause the order doesn't really matter. --- tests/bot/cogs/test_token_remover.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 518bf91ca..2ecfae2bd 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -174,8 +174,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): tokens = ["x.y.z", "a.b.c"] message = f"garbage {tokens[0]} hello {tokens[1]} world" - results = token_remover.TOKEN_RE.findall(message) - self.assertEqual(tokens, results) + results = token_remover.TOKEN_RE.finditer(message) + results = [match[0] for match in results] + self.assertCountEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- cgit v1.2.3 From 300f8c093edea03855d94be179c64c328ec842ac Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:09:04 -0700 Subject: Use real token values for testing multiple matches in regex --- tests/bot/cogs/test_token_remover.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2ecfae2bd..971bc93fc 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -171,12 +171,13 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_regex_matches_multiple_valid(self): """Should support multiple matches in the middle of a string.""" - tokens = ["x.y.z", "a.b.c"] - message = f"garbage {tokens[0]} hello {tokens[1]} world" + token_1 = "NDY3MjIzMjMwNjUwNzc3NjQx.XsyWGg.uFNEQPCc4ePwGh7egG8UicQssz8" + token_2 = "NDcyMjY1OTQzMDYyNDEzMzMy.XsyWMw.l8XPnDqb0lp-EiQ2g_0xVFT1pyc" + message = f"garbage {token_1} hello {token_2} world" results = token_remover.TOKEN_RE.finditer(message) results = [match[0] for match in results] - self.assertCountEqual(tokens, results) + self.assertCountEqual((token_1, token_2), results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- 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 d9190d997538f49c0a1b53d63a15bada3c89297f Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 27 May 2020 07:32:16 +0200 Subject: Refactor the in_whitelist deco to a check. We're moving the actual predicate into the `utils.checks` folder, just like we're doing with most of the other decorators. This is to allow us the flexibility to use it as a pure check, not only as a decorator. This commit doesn't actually change any functionality, just moves it around. --- bot/decorators.py | 54 +++-------------------------- bot/utils/checks.py | 81 ++++++++++++++++++++++++++++++++++++++++++-- tests/bot/test_decorators.py | 4 +-- 3 files changed, 86 insertions(+), 53 deletions(-) (limited to 'tests') diff --git a/bot/decorators.py b/bot/decorators.py index 306f0830c..1e77afe60 100644 --- a/bot/decorators.py +++ b/bot/decorators.py @@ -9,37 +9,20 @@ from weakref import WeakValueDictionary from discord import Colour, Embed, Member from discord.errors import NotFound from discord.ext import commands -from discord.ext.commands import CheckFailure, Cog, Context +from discord.ext.commands import Cog, Context from bot.constants import Channels, ERROR_REPLIES, RedirectOutput -from bot.utils.checks import with_role_check, without_role_check +from bot.utils.checks import in_whitelist_check, with_role_check, without_role_check log = logging.getLogger(__name__) -class InWhitelistCheckFailure(CheckFailure): - """Raised when the `in_whitelist` check fails.""" - - def __init__(self, redirect_channel: Optional[int]) -> None: - self.redirect_channel = redirect_channel - - if redirect_channel: - redirect_message = f" here. Please use the <#{redirect_channel}> channel instead" - else: - redirect_message = "" - - error_message = f"You are not allowed to use that command{redirect_message}." - - super().__init__(error_message) - - def in_whitelist( *, channels: Container[int] = (), categories: Container[int] = (), roles: Container[int] = (), redirect: Optional[int] = Channels.bot_commands, - ) -> Callable: """ Check if a command was issued in a whitelisted context. @@ -54,36 +37,9 @@ def in_whitelist( redirected to the `redirect` channel that was passed (default: #bot-commands) or simply told that they're not allowed to use this particular command (if `None` was passed). """ - if redirect and redirect not in channels: - # It does not make sense for the channel whitelist to not contain the redirection - # channel (if applicable). That's why we add the redirection channel to the `channels` - # container if it's not already in it. As we allow any container type to be passed, - # we first create a tuple in order to safely add the redirection channel. - # - # Note: It's possible for the redirect channel to be in a whitelisted category, but - # there's no easy way to check that and as a channel can easily be moved in and out of - # categories, it's probably not wise to rely on its category in any case. - channels = tuple(channels) + (redirect,) - def predicate(ctx: Context) -> bool: - """Check if a command was issued in a whitelisted context.""" - if channels and ctx.channel.id in channels: - log.trace(f"{ctx.author} may use the `{ctx.command.name}` command as they are in a whitelisted channel.") - return True - - # Only check the category id if we have a category whitelist and the channel has a `category_id` - if categories and hasattr(ctx.channel, "category_id") and ctx.channel.category_id in categories: - log.trace(f"{ctx.author} may use the `{ctx.command.name}` command as they are in a whitelisted category.") - return True - - # Only check the roles whitelist if we have one and ensure the author's roles attribute returns - # an iterable to prevent breakage in DM channels (for if we ever decide to enable commands there). - if roles and any(r.id in roles for r in getattr(ctx.author, "roles", ())): - log.trace(f"{ctx.author} may use the `{ctx.command.name}` command as they have a whitelisted role.") - return True - - log.trace(f"{ctx.author} may not use the `{ctx.command.name}` command within this context.") - raise InWhitelistCheckFailure(redirect) + """Check if command was issued in a whitelisted context.""" + return in_whitelist_check(ctx, channels, categories, roles, redirect) return commands.check(predicate) @@ -121,7 +77,7 @@ def locked() -> Callable: embed = Embed() embed.colour = Colour.red() - log.debug(f"User tried to invoke a locked command.") + log.debug("User tried to invoke a locked command.") embed.description = ( "You're already using this command. Please wait until it is done before you use it again." ) diff --git a/bot/utils/checks.py b/bot/utils/checks.py index db56c347c..63568b29e 100644 --- a/bot/utils/checks.py +++ b/bot/utils/checks.py @@ -1,12 +1,89 @@ import datetime import logging -from typing import Callable, Iterable +from typing import Callable, Container, Iterable, Optional -from discord.ext.commands import BucketType, Cog, Command, CommandOnCooldown, Context, Cooldown, CooldownMapping +from discord.ext.commands import ( + BucketType, + CheckFailure, + Cog, + Command, + CommandOnCooldown, + Context, + Cooldown, + CooldownMapping, +) + +from bot import constants log = logging.getLogger(__name__) +class InWhitelistCheckFailure(CheckFailure): + """Raised when the `in_whitelist` check fails.""" + + def __init__(self, redirect_channel: Optional[int]) -> None: + self.redirect_channel = redirect_channel + + if redirect_channel: + redirect_message = f" here. Please use the <#{redirect_channel}> channel instead" + else: + redirect_message = "" + + error_message = f"You are not allowed to use that command{redirect_message}." + + super().__init__(error_message) + + +def in_whitelist_check( + ctx: Context, + channels: Container[int] = (), + categories: Container[int] = (), + roles: Container[int] = (), + redirect: Optional[int] = constants.Channels.bot_commands, +) -> bool: + """ + Check if a command was issued in a whitelisted context. + + The whitelists that can be provided are: + + - `channels`: a container with channel ids for whitelisted channels + - `categories`: a container with category ids for whitelisted categories + - `roles`: a container with with role ids for whitelisted roles + + If the command was invoked in a context that was not whitelisted, the member is either + redirected to the `redirect` channel that was passed (default: #bot-commands) or simply + told that they're not allowed to use this particular command (if `None` was passed). + """ + if redirect and redirect not in channels: + # It does not make sense for the channel whitelist to not contain the redirection + # channel (if applicable). That's why we add the redirection channel to the `channels` + # container if it's not already in it. As we allow any container type to be passed, + # we first create a tuple in order to safely add the redirection channel. + # + # Note: It's possible for the redirect channel to be in a whitelisted category, but + # there's no easy way to check that and as a channel can easily be moved in and out of + # categories, it's probably not wise to rely on its category in any case. + channels = tuple(channels) + (redirect,) + + if channels and ctx.channel.id in channels: + log.trace(f"{ctx.author} may use the `{ctx.command.name}` command as they are in a whitelisted channel.") + return True + + # Only check the category id if we have a category whitelist and the channel has a `category_id` + if categories and hasattr(ctx.channel, "category_id") and ctx.channel.category_id in categories: + log.trace(f"{ctx.author} may use the `{ctx.command.name}` command as they are in a whitelisted category.") + return True + + # Only check the roles whitelist if we have one and ensure the author's roles attribute returns + # an iterable to prevent breakage in DM channels (for if we ever decide to enable commands there). + if roles and any(r.id in roles for r in getattr(ctx.author, "roles", ())): + log.trace(f"{ctx.author} may use the `{ctx.command.name}` command as they have a whitelisted role.") + return True + + log.trace(f"{ctx.author} may not use the `{ctx.command.name}` command within this context.") + raise InWhitelistCheckFailure(redirect) + + def with_role_check(ctx: Context, *role_ids: int) -> bool: """Returns True if the user has any one of the roles in role_ids.""" if not ctx.guild: # Return False in a DM diff --git a/tests/bot/test_decorators.py b/tests/bot/test_decorators.py index a17dd3e16..3d450caa0 100644 --- a/tests/bot/test_decorators.py +++ b/tests/bot/test_decorators.py @@ -3,10 +3,10 @@ import unittest import unittest.mock from bot import constants -from bot.decorators import InWhitelistCheckFailure, in_whitelist +from bot.decorators import in_whitelist +from bot.utils.checks import InWhitelistCheckFailure from tests import helpers - InWhitelistTestCase = collections.namedtuple("WhitelistedContextTestCase", ("kwargs", "ctx", "description")) -- cgit v1.2.3 From d310f42080278b35914bf5785fa322b97627c45f Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 27 May 2020 07:42:08 +0200 Subject: Find + change all InWhitelistCheckFailure imports --- bot/cogs/error_handler.py | 6 +++--- bot/cogs/information.py | 4 ++-- bot/cogs/verification.py | 4 ++-- tests/bot/cogs/test_information.py | 3 +-- 4 files changed, 8 insertions(+), 9 deletions(-) (limited to 'tests') diff --git a/bot/cogs/error_handler.py b/bot/cogs/error_handler.py index 23d1eed82..5de961116 100644 --- a/bot/cogs/error_handler.py +++ b/bot/cogs/error_handler.py @@ -9,7 +9,7 @@ from bot.api import ResponseCodeError from bot.bot import Bot from bot.constants import Channels from bot.converters import TagNameConverter -from bot.decorators import InWhitelistCheckFailure +from bot.utils.checks import InWhitelistCheckFailure log = logging.getLogger(__name__) @@ -166,7 +166,7 @@ class ErrorHandler(Cog): await prepared_help_command self.bot.stats.incr("errors.missing_required_argument") elif isinstance(e, errors.TooManyArguments): - await ctx.send(f"Too many arguments provided.") + await ctx.send("Too many arguments provided.") await prepared_help_command self.bot.stats.incr("errors.too_many_arguments") elif isinstance(e, errors.BadArgument): @@ -206,7 +206,7 @@ class ErrorHandler(Cog): if isinstance(e, bot_missing_errors): ctx.bot.stats.incr("errors.bot_permission_error") await ctx.send( - f"Sorry, it looks like I don't have the permissions or roles I need to do that." + "Sorry, it looks like I don't have the permissions or roles I need to do that." ) elif isinstance(e, (InWhitelistCheckFailure, errors.NoPrivateMessage)): ctx.bot.stats.incr("errors.wrong_channel_or_dm_error") diff --git a/bot/cogs/information.py b/bot/cogs/information.py index ef2f308ca..f0eb3a1ea 100644 --- a/bot/cogs/information.py +++ b/bot/cogs/information.py @@ -12,9 +12,9 @@ from discord.utils import escape_markdown from bot import constants from bot.bot import Bot -from bot.decorators import InWhitelistCheckFailure, in_whitelist, with_role +from bot.decorators import in_whitelist, with_role from bot.pagination import LinePaginator -from bot.utils.checks import cooldown_with_role_bypass, with_role_check +from bot.utils.checks import InWhitelistCheckFailure, cooldown_with_role_bypass, with_role_check from bot.utils.time import time_since log = logging.getLogger(__name__) diff --git a/bot/cogs/verification.py b/bot/cogs/verification.py index 77e8b5706..99be3cdaa 100644 --- a/bot/cogs/verification.py +++ b/bot/cogs/verification.py @@ -9,8 +9,8 @@ from discord.ext.commands import Cog, Context, command from bot import constants from bot.bot import Bot from bot.cogs.moderation import ModLog -from bot.decorators import InWhitelistCheckFailure, in_whitelist, without_role -from bot.utils.checks import without_role_check +from bot.decorators import in_whitelist, without_role +from bot.utils.checks import InWhitelistCheckFailure, without_role_check log = logging.getLogger(__name__) diff --git a/tests/bot/cogs/test_information.py b/tests/bot/cogs/test_information.py index b5f928dd6..aca6b594f 100644 --- a/tests/bot/cogs/test_information.py +++ b/tests/bot/cogs/test_information.py @@ -7,10 +7,9 @@ import discord from bot import constants from bot.cogs import information -from bot.decorators import InWhitelistCheckFailure +from bot.utils.checks import InWhitelistCheckFailure from tests import helpers - COG_PATH = "bot.cogs.information.Information" -- cgit v1.2.3 From 8e0cdb258ea6e0f25977d18336a2e07b20b5d1ee Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 27 May 2020 09:42:57 +0200 Subject: Fix failing tests related to avatar_hash --- tests/bot/cogs/sync/test_cog.py | 3 --- tests/bot/cogs/sync/test_users.py | 2 -- 2 files changed, 5 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/sync/test_cog.py b/tests/bot/cogs/sync/test_cog.py index 81398c61f..14fd909c4 100644 --- a/tests/bot/cogs/sync/test_cog.py +++ b/tests/bot/cogs/sync/test_cog.py @@ -247,14 +247,12 @@ class SyncCogListenerTests(SyncCogTestCase): before_data = { "name": "old name", "discriminator": "1234", - "avatar": "old avatar", "bot": False, } subtests = ( (True, "name", "name", "new name", "new name"), (True, "discriminator", "discriminator", "8765", 8765), - (True, "avatar", "avatar_hash", "9j2e9", "9j2e9"), (False, "bot", "bot", True, True), ) @@ -295,7 +293,6 @@ class SyncCogListenerTests(SyncCogTestCase): ) data = { - "avatar_hash": member.avatar, "discriminator": int(member.discriminator), "id": member.id, "in_guild": True, diff --git a/tests/bot/cogs/sync/test_users.py b/tests/bot/cogs/sync/test_users.py index 818883012..002a947ad 100644 --- a/tests/bot/cogs/sync/test_users.py +++ b/tests/bot/cogs/sync/test_users.py @@ -10,7 +10,6 @@ def fake_user(**kwargs): kwargs.setdefault("id", 43) kwargs.setdefault("name", "bob the test man") kwargs.setdefault("discriminator", 1337) - kwargs.setdefault("avatar_hash", None) kwargs.setdefault("roles", (666,)) kwargs.setdefault("in_guild", True) @@ -32,7 +31,6 @@ class UserSyncerDiffTests(unittest.IsolatedAsyncioTestCase): for member in members: member = member.copy() - member["avatar"] = member.pop("avatar_hash") del member["in_guild"] mock_member = helpers.MockMember(**member) -- 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 db0a384e91a463ff9668ab4f9ea5268aa332ab2d Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 27 May 2020 13:27:34 +0200 Subject: Remove the now deprecated in_channel_check. This check was no longer being used anywhere, having been replaced by in_whitelist_check. --- bot/utils/checks.py | 8 -------- tests/bot/utils/test_checks.py | 8 -------- 2 files changed, 16 deletions(-) (limited to 'tests') diff --git a/bot/utils/checks.py b/bot/utils/checks.py index d5ebe4ec9..f0ef36302 100644 --- a/bot/utils/checks.py +++ b/bot/utils/checks.py @@ -120,14 +120,6 @@ def without_role_check(ctx: Context, *role_ids: int) -> bool: return check -def in_channel_check(ctx: Context, *channel_ids: int) -> bool: - """Checks if the command was executed inside the list of specified channels.""" - check = ctx.channel.id in channel_ids - log.trace(f"{ctx.author} tried to call the '{ctx.command.name}' command. " - f"The result of the in_channel check was {check}.") - return check - - def cooldown_with_role_bypass(rate: int, per: float, type: BucketType = BucketType.default, *, bypass_roles: Iterable[int]) -> Callable: """ diff --git a/tests/bot/utils/test_checks.py b/tests/bot/utils/test_checks.py index 9610771e5..d572b6299 100644 --- a/tests/bot/utils/test_checks.py +++ b/tests/bot/utils/test_checks.py @@ -41,11 +41,3 @@ class ChecksTests(unittest.TestCase): role_id = 42 self.ctx.author.roles.append(MockRole(id=role_id)) self.assertTrue(checks.without_role_check(self.ctx, role_id + 10)) - - def test_in_channel_check_for_correct_channel(self): - self.ctx.channel.id = 42 - self.assertTrue(checks.in_channel_check(self.ctx, *[42])) - - def test_in_channel_check_for_incorrect_channel(self): - self.ctx.channel.id = 42 + 10 - self.assertFalse(checks.in_channel_check(self.ctx, *[42])) -- cgit v1.2.3 From 876fae1856f1ad876d74036899739115fd8b86c3 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 27 May 2020 13:39:32 +0200 Subject: Add some tests for `in_whitelist_check`. --- tests/bot/utils/test_checks.py | 48 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) (limited to 'tests') diff --git a/tests/bot/utils/test_checks.py b/tests/bot/utils/test_checks.py index d572b6299..de72e5748 100644 --- a/tests/bot/utils/test_checks.py +++ b/tests/bot/utils/test_checks.py @@ -1,6 +1,8 @@ import unittest +from unittest.mock import MagicMock from bot.utils import checks +from bot.utils.checks import InWhitelistCheckFailure from tests.helpers import MockContext, MockRole @@ -41,3 +43,49 @@ class ChecksTests(unittest.TestCase): role_id = 42 self.ctx.author.roles.append(MockRole(id=role_id)) self.assertTrue(checks.without_role_check(self.ctx, role_id + 10)) + + def test_in_whitelist_check_correct_channel(self): + """`in_whitelist_check` returns `True` if `Context.channel.id` is in the channel list.""" + channel_id = 3 + self.ctx.channel.id = channel_id + self.assertTrue(checks.in_whitelist_check(self.ctx, [channel_id])) + + def test_in_whitelist_check_incorrect_channel(self): + """`in_whitelist_check` raises InWhitelistCheckFailure if there's no channel match.""" + self.ctx.channel.id = 3 + with self.assertRaises(InWhitelistCheckFailure): + checks.in_whitelist_check(self.ctx, [4]) + + def test_in_whitelist_check_correct_category(self): + """`in_whitelist_check` returns `True` if `Context.channel.category_id` is in the category list.""" + category_id = 3 + self.ctx.channel.category_id = category_id + self.assertTrue(checks.in_whitelist_check(self.ctx, categories=[category_id])) + + def test_in_whitelist_check_incorrect_category(self): + """`in_whitelist_check` raises InWhitelistCheckFailure if there's no category match.""" + self.ctx.channel.category_id = 3 + with self.assertRaises(InWhitelistCheckFailure): + checks.in_whitelist_check(self.ctx, categories=[4]) + + def test_in_whitelist_check_correct_role(self): + """`in_whitelist_check` returns `True` if any of the `Context.author.roles` are in the roles list.""" + self.ctx.author.roles = (MagicMock(id=1), MagicMock(id=2)) + self.assertTrue(checks.in_whitelist_check(self.ctx, roles=[2, 6])) + + def test_in_whitelist_check_incorrect_role(self): + """`in_whitelist_check` raises InWhitelistCheckFailure if there's no role match.""" + self.ctx.author.roles = (MagicMock(id=1), MagicMock(id=2)) + with self.assertRaises(InWhitelistCheckFailure): + checks.in_whitelist_check(self.ctx, roles=[4]) + + def test_in_whitelist_check_fail_silently(self): + """`in_whitelist_check` test no exception raised if `fail_silently` is `True`""" + self.assertFalse(checks.in_whitelist_check(self.ctx, roles=[2, 6], fail_silently=True)) + + def test_in_whitelist_check_complex(self): + """`in_whitelist_check` test with multiple parameters""" + self.ctx.author.roles = (MagicMock(id=1), MagicMock(id=2)) + self.ctx.channel.category_id = 3 + self.ctx.channel.id = 5 + self.assertTrue(checks.in_whitelist_check(self.ctx, channels=[1], categories=[8], roles=[2])) -- 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 From 96db6087254c957fcb8fb45aad7ffcddb46ee839 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 17:08:18 -0700 Subject: Switch findall to finditer in assertions `find_token_in_message` now uses the latter so the tests should adjust accordingly. --- tests/bot/cogs/test_token_remover.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 971bc93fc..4fff3ab33 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -94,18 +94,18 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) - token_re.findall.assert_not_called() + token_re.finditer.assert_not_called() @autospec(TokenRemover, "is_maybe_token") @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" - token_re.findall.return_value = () + token_re.finditer.return_value = () return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) - token_re.findall.assert_called_once_with(self.msg.content) + token_re.finditer.assert_called_once_with(self.msg.content) is_maybe_token.assert_not_called() @autospec(TokenRemover, "is_maybe_token") @@ -123,7 +123,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): return_value = TokenRemover.find_token_in_message(self.msg) self.assertEqual(return_value, matches[true_index]) - token_re.findall.assert_called_once_with(self.msg.content) + token_re.finditer.assert_called_once_with(self.msg.content) # assert_has_calls isn't used cause it'd allow for extra calls before or after. # The function should short-circuit, so nothing past true_index should have been used. -- cgit v1.2.3 From f937032466a4124bacf217d1bfd0af097fc3395d Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 19:31:55 -0700 Subject: Adjust token remover tests to use the Token NamedTuple --- tests/bot/cogs/test_token_remover.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 4fff3ab33..65bc1ee58 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -7,7 +7,7 @@ from discord import Colour from bot import constants from bot.cogs import token_remover from bot.cogs.moderation import ModLog -from bot.cogs.token_remover import TokenRemover +from bot.cogs.token_remover import Token, TokenRemover from tests.helpers import MockBot, MockMessage, autospec @@ -224,17 +224,19 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "LOG_MESSAGE") def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" + token = Token("NDY3MjIzMjMwNjUwNzc3NjQx", "XsySD_", "s45jqDV_Iisn-symw0yDRrk_jf4") log_message.format.return_value = "Howdy" - return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") + + return_value = TokenRemover.format_log_message(self.msg, token) self.assertEqual(return_value, log_message.format.return_value) log_message.format.assert_called_once_with( author=self.msg.author, author_id=self.msg.author.id, channel=self.msg.channel.mention, - user_id="MTIz", - timestamp="DN9R_A", - hmac="xxx", + user_id=token.user_id, + timestamp=token.timestamp, + hmac="x" * len(token.hmac), ) @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) @@ -244,7 +246,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): """Should delete the message and send a mod log.""" cog = TokenRemover(self.bot) mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) - token = "MTIz.DN9R_A.xyz" + token = mock.create_autospec(Token, spec_set=True, instance=True) log_msg = "testing123" mod_log_property.return_value = mod_log -- cgit v1.2.3 From 12b8f5002807144451a313180c639bf6b4925f2e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 20:00:33 -0700 Subject: Add more thorough and realistic inputs for token ID and timestamp tests The tests for valid inputs and invalid inputs were split to make them more readable. --- tests/bot/cogs/test_token_remover.py | 70 ++++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 18 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 65bc1ee58..ffe76865a 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -24,31 +24,65 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) self.msg.author.avatar_url_as.return_value = "picture-lemon.png" - def test_is_valid_user_id(self): - """Should correctly discern valid user IDs and ignore non-numeric and non-ASCII IDs.""" - subtests = ( - ("MTIz", True), # base64(123) - ("YWJj", False), # base64(abc) - ("λδµ", False), + def test_is_valid_user_id_valid(self): + """Should consider user IDs valid if they decode entirely to ASCII digits.""" + ids = ( + "NDcyMjY1OTQzMDYyNDEzMzMy", + "NDc1MDczNjI5Mzk5NTQ3OTA0", + "NDY3MjIzMjMwNjUwNzc3NjQx", ) - for user_id, is_valid in subtests: - with self.subTest(user_id=user_id, is_valid=is_valid): + for user_id in ids: + with self.subTest(user_id=user_id): result = TokenRemover.is_valid_user_id(user_id) - self.assertIs(result, is_valid) + self.assertTrue(result) + + def test_is_valid_user_id_invalid(self): + """Should consider non-digit and non-ASCII IDs invalid.""" + ids = ( + ("SGVsbG8gd29ybGQ", "non-digit ASCII"), + ("0J_RgNC40LLQtdGCINC80LjRgA", "cyrillic text"), + ("4pO14p6L4p6C4pG34p264pGl8J-EiOKSj-KCieKBsA", "Unicode digits"), + ("4oaA4oaB4oWh4oWi4Lyz4Lyq4Lyr4LG9", "Unicode numerals"), + ("8J2fjvCdn5nwnZ-k8J2fr_Cdn7rgravvvJngr6c", "Unicode decimals"), + ("{hello}[world]&(bye!)", "ASCII invalid Base64"), + ("Þíß-ï§-ňøẗ-våłìÐ", "Unicode invalid Base64"), + ) - def test_is_valid_timestamp(self): - """Should correctly discern valid timestamps.""" - subtests = ( - ("DN9r_A", True), - ("MTIz", False), # base64(123) - ("λδµ", False), + for user_id, msg in ids: + with self.subTest(msg=msg): + result = TokenRemover.is_valid_user_id(user_id) + self.assertFalse(result) + + def test_is_valid_timestamp_valid(self): + """Should consider timestamps valid if they're greater than the Discord epoch.""" + timestamps = ( + "XsyRkw", + "Xrim9Q", + "XsyR-w", + "XsySD_", + "Dn9r_A", + ) + + for timestamp in timestamps: + with self.subTest(timestamp=timestamp): + result = TokenRemover.is_valid_timestamp(timestamp) + self.assertTrue(result) + + def test_is_valid_timestamp_invalid(self): + """Should consider timestamps invalid if they're before Discord epoch or can't be parsed.""" + timestamps = ( + ("B4Yffw", "DISCORD_EPOCH - TOKEN_EPOCH - 1"), + ("ew", "123"), + ("AoIKgA", "42076800"), + ("{hello}[world]&(bye!)", "ASCII invalid Base64"), + ("Þíß-ï§-ňøẗ-våłìÐ", "Unicode invalid Base64"), ) - for timestamp, is_valid in subtests: - with self.subTest(timestamp=timestamp, is_valid=is_valid): + for timestamp, msg in timestamps: + with self.subTest(msg=msg): result = TokenRemover.is_valid_timestamp(timestamp) - self.assertIs(result, is_valid) + self.assertFalse(result) def test_mod_log_property(self): """The `mod_log` property should ask the bot to return the `ModLog` cog.""" -- cgit v1.2.3 From 67472080fef5c38b21d74daa2178c3f35081b58f Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 19:52:41 -0700 Subject: Remove is_maybe_token tests The function was removed due to redundancy. Therefore, its tests are obsolete. --- tests/bot/cogs/test_token_remover.py | 33 --------------------------------- 1 file changed, 33 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index ffe76865a..5dd12636c 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -213,39 +213,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = [match[0] for match in results] self.assertCountEqual((token_1, token_2), results) - @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") - def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): - """False should be returned for tokens which do not have all 3 parts.""" - return_value = TokenRemover.is_maybe_token("x.y") - - self.assertFalse(return_value) - valid_user.assert_not_called() - valid_time.assert_not_called() - - @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") - def test_is_maybe_token(self, valid_user, valid_time): - """Should return True if the user ID and timestamp are valid or return False otherwise.""" - subtests = ( - (False, True, False), - (True, False, False), - (True, True, True), - ) - - for user_return, time_return, expected in subtests: - valid_user.reset_mock() - valid_time.reset_mock() - - with self.subTest(user_return=user_return, time_return=time_return, expected=expected): - valid_user.return_value = user_return - valid_time.return_value = time_return - - actual = TokenRemover.is_maybe_token("x.y.z") - self.assertIs(actual, expected) - - valid_user.assert_called_once_with("x") - if user_return: - valid_time.assert_called_once_with("y") - async def test_delete_message(self): """The message should be deleted, and a message should be sent to the same channel.""" await TokenRemover.delete_message(self.msg) -- cgit v1.2.3 From 84cd8235863acc80b7f140309424c33180cc34ea Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 20:32:48 -0700 Subject: Adjust find_token_in_message tests for the recent cog changes It now supports the changes that switched to finditer, added match groups, and added the Token NamedTuple. It also accounts for the is_maybe_token function being removed. For the sake of simplicity, call assertions on is_valid_user_id and is_valid_timestamp were not made. --- tests/bot/cogs/test_token_remover.py | 39 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5dd12636c..8238e235a 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,4 +1,5 @@ import unittest +from re import Match from unittest import mock from unittest.mock import MagicMock @@ -130,9 +131,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.finditer.assert_not_called() - @autospec(TokenRemover, "is_maybe_token") @autospec("bot.cogs.token_remover", "TOKEN_RE") - def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): + def test_find_token_no_matches(self, token_re): """None should be returned if the regex matches no tokens in a message.""" token_re.finditer.return_value = () @@ -140,30 +140,31 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.finditer.assert_called_once_with(self.msg.content) - is_maybe_token.assert_not_called() - @autospec(TokenRemover, "is_maybe_token") + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + @autospec("bot.cogs.token_remover", "Token") @autospec("bot.cogs.token_remover", "TOKEN_RE") - def test_find_token_returns_found_token(self, token_re, is_maybe_token): - """The found token should be returned.""" - true_index = 1 - matches = ("foo", "bar", "baz") - side_effects = [False] * len(matches) - side_effects[true_index] = True - - token_re.findall.return_value = matches - is_maybe_token.side_effect = side_effects + def test_find_token_valid_match(self, token_re, token_cls, is_valid_id, is_valid_timestamp): + """The first match with a valid user ID and timestamp should be returned as a `Token`.""" + matches = [ + mock.create_autospec(Match, spec_set=True, instance=True), + mock.create_autospec(Match, spec_set=True, instance=True), + ] + tokens = [ + mock.create_autospec(Token, spec_set=True, instance=True), + mock.create_autospec(Token, spec_set=True, instance=True), + ] + + token_re.finditer.return_value = matches + token_cls.side_effect = tokens + is_valid_id.side_effect = (False, True) # The 1st match will be invalid, 2nd one valid. + is_valid_timestamp.return_value = True return_value = TokenRemover.find_token_in_message(self.msg) - self.assertEqual(return_value, matches[true_index]) + self.assertEqual(tokens[1], return_value) token_re.finditer.assert_called_once_with(self.msg.content) - # assert_has_calls isn't used cause it'd allow for extra calls before or after. - # The function should short-circuit, so nothing past true_index should have been used. - calls = [mock.call(match) for match in matches[:true_index + 1]] - self.assertEqual(is_maybe_token.mock_calls, calls) - def test_regex_invalid_tokens(self): """Messages without anything looking like a token are not matched.""" tokens = ( -- cgit v1.2.3 From 5930a044b8347019d474a809fc86f89263574ad0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 20:33:34 -0700 Subject: Test find_token_in_message returns None for invalid matches This covers the case when a token is matched, but its user ID and timestamp turn out to be invalid. --- tests/bot/cogs/test_token_remover.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 8238e235a..9b4b04ecd 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -165,6 +165,21 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(tokens[1], return_value) token_re.finditer.assert_called_once_with(self.msg.content) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + @autospec("bot.cogs.token_remover", "Token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_invalid_matches(self, token_re, token_cls, is_valid_id, is_valid_timestamp): + """None should be returned if no matches have valid user IDs or timestamps.""" + token_re.finditer.return_value = [mock.create_autospec(Match, spec_set=True, instance=True)] + token_cls.return_value = mock.create_autospec(Token, spec_set=True, instance=True) + is_valid_id.return_value = False + is_valid_timestamp.return_value = False + + return_value = TokenRemover.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.finditer.assert_called_once_with(self.msg.content) + def test_regex_invalid_tokens(self): """Messages without anything looking like a token are not matched.""" tokens = ( -- cgit v1.2.3 From f59e63454ffa582765847e8a26d9d97dcd9ff7b2 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 30 May 2020 01:42:02 +0200 Subject: Fix busted test_information test. I wish this test didn't exist. --- tests/bot/cogs/test_information.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_information.py b/tests/bot/cogs/test_information.py index aca6b594f..79c0e0ad3 100644 --- a/tests/bot/cogs/test_information.py +++ b/tests/bot/cogs/test_information.py @@ -148,14 +148,18 @@ class InformationCogTests(unittest.TestCase): Voice region: {self.ctx.guild.region} Features: {', '.join(self.ctx.guild.features)} - **Counts** - Members: {self.ctx.guild.member_count:,} - Roles: {len(self.ctx.guild.roles)} + **Channel counts** Category channels: 1 Text channels: 1 Voice channels: 1 + Staff channels: 0 + + **Member counts** + Members: {self.ctx.guild.member_count:,} + Staff members: 0 + Roles: {len(self.ctx.guild.roles)} - **Members** + **Member statuses** {constants.Emojis.status_online} 2 {constants.Emojis.status_idle} 1 {constants.Emojis.status_dnd} 4 -- cgit v1.2.3 From 96b026198a4ca2074f4fd7ea68e8a09acd5b38e4 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 30 May 2020 09:34:39 +0300 Subject: Simplify infraction reason truncation tests --- tests/bot/cogs/moderation/test_infractions.py | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py index 5548d9f68..ad3c95958 100644 --- a/tests/bot/cogs/moderation/test_infractions.py +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -27,15 +27,14 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): self.cog.apply_infraction = AsyncMock() self.bot.get_cog.return_value = AsyncMock() self.cog.mod_log.ignore = Mock() + self.ctx.guild.ban = Mock() await self.cog.apply_ban(self.ctx, self.target, "foo bar" * 3000) - ban = self.cog.apply_infraction.call_args[0][3] - self.assertEqual( - ban.cr_frame.f_locals["kwargs"]["reason"], - textwrap.shorten("foo bar" * 3000, 512, placeholder="...") + self.ctx.guild.ban.assert_called_once_with( + self.target, + reason=textwrap.shorten("foo bar" * 3000, 512, placeholder="..."), + delete_message_days=0 ) - # Await ban to avoid not awaited coroutine warning - await ban @patch("bot.cogs.moderation.utils.post_infraction") async def test_apply_kick_reason_truncation(self, post_infraction_mock): @@ -44,12 +43,7 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): self.cog.apply_infraction = AsyncMock() self.cog.mod_log.ignore = Mock() + self.target.kick = Mock() await self.cog.apply_kick(self.ctx, self.target, "foo bar" * 3000) - kick = self.cog.apply_infraction.call_args[0][3] - self.assertEqual( - kick.cr_frame.f_locals["kwargs"]["reason"], - textwrap.shorten("foo bar" * 3000, 512, placeholder="...") - ) - # Await kick to avoid not awaited coroutine warning - await kick + self.target.kick.assert_called_once_with(reason=textwrap.shorten("foo bar" * 3000, 512, placeholder="...")) -- cgit v1.2.3 From 323317496310ef474a39d468e273703106e44768 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 30 May 2020 10:07:21 +0300 Subject: Infr. Tests: Add `apply_infraction` awaiting assertion with args --- tests/bot/cogs/moderation/test_infractions.py | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_infractions.py b/tests/bot/cogs/moderation/test_infractions.py index ad3c95958..da4e92ccc 100644 --- a/tests/bot/cogs/moderation/test_infractions.py +++ b/tests/bot/cogs/moderation/test_infractions.py @@ -35,6 +35,9 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): reason=textwrap.shorten("foo bar" * 3000, 512, placeholder="..."), delete_message_days=0 ) + self.cog.apply_infraction.assert_awaited_once_with( + self.ctx, {"foo": "bar"}, self.target, self.ctx.guild.ban.return_value + ) @patch("bot.cogs.moderation.utils.post_infraction") async def test_apply_kick_reason_truncation(self, post_infraction_mock): @@ -47,3 +50,6 @@ class TruncationTests(unittest.IsolatedAsyncioTestCase): await self.cog.apply_kick(self.ctx, self.target, "foo bar" * 3000) self.target.kick.assert_called_once_with(reason=textwrap.shorten("foo bar" * 3000, 512, placeholder="...")) + self.cog.apply_infraction.assert_awaited_once_with( + self.ctx, {"foo": "bar"}, self.target, self.target.kick.return_value + ) -- cgit v1.2.3 From 876b4846f612fe0011cc2e0b498b4df9e54d74cb Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 31 May 2020 19:17:07 +0200 Subject: Add support for bool values in RedisCache We're gonna need this for the help channel handling, and it seems like a reasonable type to support anyway. It requires a tiny bit of special handling, but nothing outrageous. --- bot/utils/redis_cache.py | 14 ++++++++++++-- tests/bot/utils/test_redis_cache.py | 4 +++- 2 files changed, 15 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index de80cee84..2926e7a89 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -2,6 +2,7 @@ from __future__ import annotations import asyncio import logging +from distutils.util import strtobool from functools import partialmethod from typing import Any, Dict, ItemsView, Optional, Tuple, Union @@ -11,7 +12,7 @@ log = logging.getLogger(__name__) # Type aliases RedisKeyType = Union[str, int] -RedisValueType = Union[str, int, float] +RedisValueType = Union[str, int, float, bool] RedisKeyOrValue = Union[RedisKeyType, RedisValueType] # Prefix tuples @@ -20,6 +21,7 @@ _VALUE_PREFIXES = ( ("f|", float), ("i|", int), ("s|", str), + ("b|", bool), ) _KEY_PREFIXES = ( ("i|", int), @@ -117,7 +119,8 @@ class RedisCache: def _to_typestring(key_or_value: RedisKeyOrValue, prefixes: _PrefixTuple) -> str: """Turn a valid Redis type into a typestring.""" for prefix, _type in prefixes: - if isinstance(key_or_value, _type): + # isinstance is a bad idea here, because isintance(False, int) == True. + if type(key_or_value) is _type: return f"{prefix}{key_or_value}" raise TypeError(f"RedisCache._to_typestring only supports the following: {prefixes}.") @@ -131,6 +134,13 @@ class RedisCache: # Now we convert our unicode string back into the type it originally was. for prefix, _type in prefixes: if key_or_value.startswith(prefix): + + # For booleans, we need special handling because bool("False") is True. + if prefix == "b|": + value = key_or_value[len(prefix):] + return bool(strtobool(value)) + + # Otherwise we can just convert normally. return _type(key_or_value[len(prefix):]) raise TypeError(f"RedisCache._from_typestring only supports the following: {prefixes}.") diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 8c1a40640..62c411681 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -59,7 +59,9 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): test_cases = ( ('favorite_fruit', 'melon'), ('favorite_number', 86), - ('favorite_fraction', 86.54) + ('favorite_fraction', 86.54), + ('favorite_boolean', False), + ('other_boolean', True), ) # Test that we can get and set different types. -- cgit v1.2.3 From ebbaa6274cfc278c772593b193356aa8bf066de4 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 31 May 2020 14:17:20 -0700 Subject: Remove redis namespace collision test --- tests/bot/utils/test_redis_cache.py | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'tests') diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 8c1a40640..e5d6e4078 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -44,16 +44,6 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): with self.assertRaises(RuntimeError): await bad_cache.set("test", "me_up_deadman") - def test_namespace_collision(self): - """Test that we prevent colliding namespaces.""" - 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.""" test_cases = ( -- cgit v1.2.3 From 9b3ab7df5ae1ecf95705f2fab7d99fdb36eb98ea Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 2 Jun 2020 19:22:49 -0700 Subject: Token remover: remove the `delete_message` function It's redundant; there's no benefit here in abstracting two lines of code into a function. --- bot/cogs/token_remover.py | 9 ++------- tests/bot/cogs/test_token_remover.py | 19 +++++++------------ 2 files changed, 9 insertions(+), 19 deletions(-) (limited to 'tests') diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 46329e207..d55e079e9 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -79,7 +79,8 @@ class TokenRemover(Cog): async def take_action(self, msg: Message, found_token: Token) -> None: """Remove the `msg` containing the `found_token` and send a mod log message.""" self.mod_log.ignore(Event.message_delete, msg.id) - await self.delete_message(msg) + await msg.delete() + await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) log_message = self.format_log_message(msg, found_token) log.debug(log_message) @@ -96,12 +97,6 @@ class TokenRemover(Cog): self.bot.stats.incr("tokens.removed_tokens") - @staticmethod - async def delete_message(msg: Message) -> None: - """Remove a `msg` containing a token and send an explanatory message in the same channel.""" - await msg.delete() - await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) - @staticmethod def format_log_message(msg: Message, token: Token) -> str: """Return the log message to send for `token` being censored in `msg`.""" diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 9b4b04ecd..a10124d2d 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -229,15 +229,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = [match[0] for match in results] self.assertCountEqual((token_1, token_2), results) - async def test_delete_message(self): - """The message should be deleted, and a message should be sent to the same channel.""" - await TokenRemover.delete_message(self.msg) - - self.msg.delete.assert_called_once_with() - self.msg.channel.send.assert_called_once_with( - token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) - ) - @autospec("bot.cogs.token_remover", "LOG_MESSAGE") def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" @@ -258,8 +249,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) @autospec("bot.cogs.token_remover", "log") - @autospec(TokenRemover, "delete_message", "format_log_message") - async def test_take_action(self, delete_message, format_log_message, logger, mod_log_property): + @autospec(TokenRemover, "format_log_message") + async def test_take_action(self, format_log_message, logger, mod_log_property): """Should delete the message and send a mod log.""" cog = TokenRemover(self.bot) mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) @@ -271,7 +262,11 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): await cog.take_action(self.msg, token) - delete_message.assert_awaited_once_with(self.msg) + self.msg.delete.assert_called_once_with() + self.msg.channel.send.assert_called_once_with( + token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + ) + format_log_message.assert_called_once_with(self.msg, token) logger.debug.assert_called_with(log_msg) self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") -- cgit v1.2.3 From be4902cbd66c2f7223608ddbfee4aa4f0e1a011a Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 22:11:53 +0200 Subject: Test for channel not silenced message --- tests/bot/cogs/moderation/test_silence.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_silence.py b/tests/bot/cogs/moderation/test_silence.py index 3fd149f04..ab3d0742a 100644 --- a/tests/bot/cogs/moderation/test_silence.py +++ b/tests/bot/cogs/moderation/test_silence.py @@ -127,10 +127,20 @@ class SilenceTests(unittest.IsolatedAsyncioTestCase): self.ctx.reset_mock() async def test_unsilence_sent_correct_discord_message(self): - """Proper reply after a successful unsilence.""" - with mock.patch.object(self.cog, "_unsilence", return_value=True): - await self.cog.unsilence.callback(self.cog, self.ctx) - self.ctx.send.assert_called_once_with(f"{Emojis.check_mark} unsilenced current channel.") + """Check if proper message was sent when unsilencing channel.""" + test_cases = ( + (True, f"{Emojis.check_mark} unsilenced current channel."), + (False, f"{Emojis.cross_mark} current channel was not silenced.") + ) + for _unsilence_patch_return, result_message in test_cases: + with self.subTest( + starting_silenced_state=_unsilence_patch_return, + result_message=result_message + ): + with mock.patch.object(self.cog, "_unsilence", return_value=_unsilence_patch_return): + await self.cog.unsilence.callback(self.cog, self.ctx) + self.ctx.send.assert_called_once_with(result_message) + self.ctx.reset_mock() async def test_silence_private_for_false(self): """Permissions are not set and `False` is returned in an already silenced channel.""" -- cgit v1.2.3 From c7373fa1143a2d2f2d784a59d40bcb40ee765bfb Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 15 Jun 2020 10:26:23 -0700 Subject: Token remover: ignore DMs It's a private channel so there's no risk of a token "leaking". Furthermore, messages cannot be deleted in DMs. --- bot/cogs/token_remover.py | 3 +++ tests/bot/cogs/test_token_remover.py | 10 ++++++++++ 2 files changed, 13 insertions(+) (limited to 'tests') diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index d55e079e9..493479df9 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -63,6 +63,9 @@ class TokenRemover(Cog): See: https://discordapp.com/developers/docs/reference#snowflakes """ + if not msg.guild: + return # Ignore DMs; can't delete messages in there anyway. + found_token = self.find_token_in_message(msg) if found_token: await self.take_action(msg, found_token) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index a10124d2d..22c31d7b1 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -121,6 +121,16 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): find_token_in_message.assert_called_once_with(self.msg) take_action.assert_not_awaited() + @autospec(TokenRemover, "find_token_in_message") + async def test_on_message_ignores_dms(self, find_token_in_message): + """Shouldn't parse a message if it is a DM.""" + cog = TokenRemover(self.bot) + self.msg.guild = None + + await cog.on_message(self.msg) + + find_token_in_message.assert_not_called() + @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_ignores_bot_messages(self, token_re): """The token finder should ignore messages authored by bots.""" -- cgit v1.2.3 From 2fa7429327e787a65803c16609da21463723bfeb Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 15 Jun 2020 10:38:46 -0700 Subject: Token remover: move bot check to on_message It just makes more sense to me to filter out messages at an earlier stage. --- bot/cogs/token_remover.py | 8 +++----- tests/bot/cogs/test_token_remover.py | 23 +++++++---------------- 2 files changed, 10 insertions(+), 21 deletions(-) (limited to 'tests') diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 493479df9..1f7517501 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -63,8 +63,9 @@ class TokenRemover(Cog): See: https://discordapp.com/developers/docs/reference#snowflakes """ - if not msg.guild: - return # Ignore DMs; can't delete messages in there anyway. + # Ignore DMs; can't delete messages in there anyway. + if not msg.guild or msg.author.bot: + return found_token = self.find_token_in_message(msg) if found_token: @@ -115,9 +116,6 @@ class TokenRemover(Cog): @classmethod def find_token_in_message(cls, msg: Message) -> t.Optional[Token]: """Return a seemingly valid token found in `msg` or `None` if no token is found.""" - if msg.author.bot: - return - # Use finditer rather than search to guard against method calls prematurely returning the # token check (e.g. `message.channel.send` also matches our token pattern) for match in TOKEN_RE.finditer(msg.content): diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 22c31d7b1..98ea9f823 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -122,24 +122,15 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): take_action.assert_not_awaited() @autospec(TokenRemover, "find_token_in_message") - async def test_on_message_ignores_dms(self, find_token_in_message): - """Shouldn't parse a message if it is a DM.""" + async def test_on_message_ignores_dms_bots(self, find_token_in_message): + """Shouldn't parse a message if it is a DM or authored by a bot.""" cog = TokenRemover(self.bot) - self.msg.guild = None + dm_msg = MockMessage(guild=None) + bot_msg = MockMessage(author=MagicMock(bot=True)) - await cog.on_message(self.msg) - - find_token_in_message.assert_not_called() - - @autospec("bot.cogs.token_remover", "TOKEN_RE") - def test_find_token_ignores_bot_messages(self, token_re): - """The token finder should ignore messages authored by bots.""" - self.msg.author.bot = True - - return_value = TokenRemover.find_token_in_message(self.msg) - - self.assertIsNone(return_value) - token_re.finditer.assert_not_called() + for msg in (dm_msg, bot_msg): + await cog.on_message(msg) + find_token_in_message.assert_not_called() @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_no_matches(self, token_re): -- cgit v1.2.3 From 3aecf14419c87e533d47fe082abeb54ca9edb73c Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 15 Jun 2020 10:49:18 -0700 Subject: Token remover: exit early if message already deleted --- bot/cogs/token_remover.py | 10 ++++++++-- tests/bot/cogs/test_token_remover.py | 15 ++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 1f7517501..ef979f222 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -4,7 +4,7 @@ import logging import re import typing as t -from discord import Colour, Message +from discord import Colour, Message, NotFound from discord.ext.commands import Cog from bot import utils @@ -83,7 +83,13 @@ class TokenRemover(Cog): async def take_action(self, msg: Message, found_token: Token) -> None: """Remove the `msg` containing the `found_token` and send a mod log message.""" self.mod_log.ignore(Event.message_delete, msg.id) - await msg.delete() + + try: + await msg.delete() + except NotFound: + log.debug(f"Failed to remove token in message {msg.id}: message already deleted.") + return + await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) log_message = self.format_log_message(msg, found_token) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 98ea9f823..3349caa73 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -3,7 +3,7 @@ from re import Match from unittest import mock from unittest.mock import MagicMock -from discord import Colour +from discord import Colour, NotFound from bot import constants from bot.cogs import token_remover @@ -282,6 +282,19 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): channel_id=constants.Channels.mod_alerts ) + @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) + async def test_take_action_delete_failure(self, mod_log_property): + """Shouldn't send any messages if the token message can't be deleted.""" + cog = TokenRemover(self.bot) + mod_log_property.return_value = mock.create_autospec(ModLog, spec_set=True, instance=True) + self.msg.delete.side_effect = NotFound(MagicMock(), MagicMock()) + + token = mock.create_autospec(Token, spec_set=True, instance=True) + await cog.take_action(self.msg, token) + + self.msg.delete.assert_called_once_with() + self.msg.channel.send.assert_not_awaited() + class TokenRemoverExtensionTests(unittest.TestCase): """Tests for the token_remover extension.""" -- cgit v1.2.3 From 581573f2ece96a9ec666795431ff21068e949a63 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 20 Jun 2020 01:20:35 +0200 Subject: Write unit test for `sub_clyde` --- tests/bot/utils/test_messages.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 tests/bot/utils/test_messages.py (limited to 'tests') diff --git a/tests/bot/utils/test_messages.py b/tests/bot/utils/test_messages.py new file mode 100644 index 000000000..9c22c9751 --- /dev/null +++ b/tests/bot/utils/test_messages.py @@ -0,0 +1,27 @@ +import unittest + +from bot.utils import messages + + +class TestMessages(unittest.TestCase): + """Tests for functions in the `bot.utils.messages` module.""" + + def test_sub_clyde(self): + """Uppercase E's and lowercase e's are substituted with their cyrillic counterparts.""" + sub_e = "\u0435" + sub_E = "\u0415" # noqa: N806: Uppercase E in variable name + + test_cases = ( + (None, None), + ("", ""), + ("clyde", f"clyd{sub_e}"), + ("CLYDE", f"CLYD{sub_E}"), + ("cLyDe", f"cLyD{sub_e}"), + ("BIGclyde", f"BIGclyd{sub_e}"), + ("small clydeus the unholy", f"small clyd{sub_e}us the unholy"), + ("BIGCLYDE, babyclyde", f"BIGCLYD{sub_E}, babyclyd{sub_e}"), + ) + + for username_in, username_out in test_cases: + with self.subTest(input=username_in, expected_output=username_out): + self.assertEqual(messages.sub_clyde(username_in), username_out) -- cgit v1.2.3