From 4890cc5ba43ad73229ce4d2fe240acaf39194edb Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Tue, 14 Apr 2020 08:30:18 +0300 Subject: Created tests for `bot.cogs.logging` connected message. --- tests/bot/cogs/test_logging.py | 42 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 tests/bot/cogs/test_logging.py (limited to 'tests') diff --git a/tests/bot/cogs/test_logging.py b/tests/bot/cogs/test_logging.py new file mode 100644 index 000000000..ba98a5a56 --- /dev/null +++ b/tests/bot/cogs/test_logging.py @@ -0,0 +1,42 @@ +import unittest +from unittest.mock import patch + +from bot import constants +from bot.cogs.logging import Logging +from tests.helpers import MockBot, MockTextChannel + + +class LoggingTests(unittest.IsolatedAsyncioTestCase): + """Test cases for connected login.""" + + def setUp(self): + self.bot = MockBot() + self.cog = Logging(self.bot) + self.dev_log = MockTextChannel(id=1234, name="dev-log") + + @patch("bot.cogs.logging.DEBUG_MODE", False) + async def test_debug_mode_false(self): + """Should send connected message to dev-log.""" + self.bot.get_channel.return_value = self.dev_log + + await self.cog.startup_greeting() + self.bot.wait_until_guild_available.assert_awaited_once_with() + self.bot.get_channel.assert_called_once_with(constants.Channels.dev_log) + + embed = self.dev_log.send.call_args[1]['embed'] + self.dev_log.send.assert_awaited_once_with(embed=embed) + + self.assertEqual(embed.description, "Connected!") + self.assertEqual(embed.author.name, "Python Bot") + self.assertEqual(embed.author.url, "https://github.com/python-discord/bot") + self.assertEqual( + embed.author.icon_url, + "https://raw.githubusercontent.com/python-discord/branding/master/logos/logo_circle/logo_circle_large.png" + ) + + @patch("bot.cogs.logging.DEBUG_MODE", True) + async def test_debug_mode_true(self): + """Should not send anything to dev-log.""" + await self.cog.startup_greeting() + self.bot.wait_until_guild_available.assert_awaited_once_with() + self.bot.get_channel.assert_not_called() -- cgit v1.2.3 From ededd1879cfb914445342b202d4c66aed23ee94b Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Fri, 22 May 2020 08:43:10 +0300 Subject: Logging Tests: Simplify `DEBUG_MODE` `False` test - Remove embed attributes checks - Replace `self.dev_log.assert_awaited_once_with` with `self.dev_log.assert_awaited_once`. --- tests/bot/cogs/test_logging.py | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_logging.py b/tests/bot/cogs/test_logging.py index ba98a5a56..8a18fdcd6 100644 --- a/tests/bot/cogs/test_logging.py +++ b/tests/bot/cogs/test_logging.py @@ -22,17 +22,7 @@ class LoggingTests(unittest.IsolatedAsyncioTestCase): await self.cog.startup_greeting() self.bot.wait_until_guild_available.assert_awaited_once_with() self.bot.get_channel.assert_called_once_with(constants.Channels.dev_log) - - embed = self.dev_log.send.call_args[1]['embed'] - self.dev_log.send.assert_awaited_once_with(embed=embed) - - self.assertEqual(embed.description, "Connected!") - self.assertEqual(embed.author.name, "Python Bot") - self.assertEqual(embed.author.url, "https://github.com/python-discord/bot") - self.assertEqual( - embed.author.icon_url, - "https://raw.githubusercontent.com/python-discord/branding/master/logos/logo_circle/logo_circle_large.png" - ) + self.dev_log.send.assert_awaited_once() @patch("bot.cogs.logging.DEBUG_MODE", True) async def test_debug_mode_true(self): -- cgit v1.2.3 From 5f5a51b1715228ac5b401ef6bed8a83491e313de Mon Sep 17 00:00:00 2001 From: Kyle Stanley Date: Thu, 4 Jun 2020 03:17:11 -0400 Subject: Improve LinePaginator to support long lines --- bot/cogs/moderation/management.py | 8 ++--- bot/pagination.py | 66 +++++++++++++++++++++++++++++++++++---- tests/bot/test_pagination.py | 41 +++++++++++++++++++----- 3 files changed, 98 insertions(+), 17 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/management.py b/bot/cogs/moderation/management.py index 250a24247..ad17a90b0 100644 --- a/bot/cogs/moderation/management.py +++ b/bot/cogs/moderation/management.py @@ -83,14 +83,14 @@ class ModManagement(commands.Cog): "actor__id": ctx.author.id, "ordering": "-inserted_at" } - infractions = await self.bot.api_client.get(f"bot/infractions", params=params) + infractions = await self.bot.api_client.get("bot/infractions", params=params) if infractions: old_infraction = infractions[0] infraction_id = old_infraction["id"] else: await ctx.send( - f":x: Couldn't find most recent infraction; you have never given an infraction." + ":x: Couldn't find most recent infraction; you have never given an infraction." ) return else: @@ -224,7 +224,7 @@ class ModManagement(commands.Cog): ) -> None: """Send a paginated embed of infractions for the specified user.""" if not infractions: - await ctx.send(f":warning: No infractions could be found for that query.") + await ctx.send(":warning: No infractions could be found for that query.") return lines = tuple( @@ -268,12 +268,12 @@ class ModManagement(commands.Cog): User: {self.bot.get_user(user_id)} (`{user_id}`) Type: **{infraction["type"]}** Shadow: {hidden} - Reason: {infraction["reason"] or "*None*"} Created: {created} Expires: {expires} Remaining: {remaining} Actor: {actor.mention if actor else actor_id} ID: `{infraction["id"]}` + Reason: {infraction["reason"] or "*None*"} {"**===============**" if active else "==============="} """) diff --git a/bot/pagination.py b/bot/pagination.py index 90c8f849c..5c7be564d 100644 --- a/bot/pagination.py +++ b/bot/pagination.py @@ -37,12 +37,19 @@ class LinePaginator(Paginator): The suffix appended at the end of every page. e.g. three backticks. * max_size: `int` The maximum amount of codepoints allowed in a page. + * scale_to_size: `int` + The maximum amount of characters a single line can scale up to. * max_lines: `int` The maximum amount of lines allowed in a page. """ def __init__( - self, prefix: str = '```', suffix: str = '```', max_size: int = 2000, max_lines: int = None + self, + prefix: str = '```', + suffix: str = '```', + max_size: int = 2000, + scale_to_size: int = 2000, + max_lines: t.Optional[int] = None ) -> None: """ This function overrides the Paginator.__init__ from inside discord.ext.commands. @@ -52,6 +59,10 @@ class LinePaginator(Paginator): self.prefix = prefix self.suffix = suffix self.max_size = max_size - len(suffix) + if scale_to_size < max_size: + raise ValueError("scale_to_size must be >= max_size.") + + self.scale_to_size = scale_to_size self.max_lines = max_lines self._current_page = [prefix] self._linecount = 0 @@ -62,14 +73,26 @@ class LinePaginator(Paginator): """ Adds a line to the current page. - If the line exceeds the `self.max_size` then an exception is raised. + If the line exceeds `self.max_size`, then `self.max_size` will go up to `scale_to_size` for + a single line before creating a new page. If it is still exceeded, the excess characters + are stored and placed on the next pages until there are none remaining (by word boundary). + + Raises a RuntimeError if `self.max_size` is still exceeded after attempting to continue + onto the next page. This function overrides the `Paginator.add_line` from inside `discord.ext.commands`. It overrides in order to allow us to configure the maximum number of lines per page. """ - if len(line) > self.max_size - len(self.prefix) - 2: - raise RuntimeError('Line exceeds maximum page size %s' % (self.max_size - len(self.prefix) - 2)) + remaining_words = None + if len(line) > (max_chars := self.max_size - len(self.prefix) - 2): + if len(line) > self.scale_to_size: + line, remaining_words = self._split_remaining_words(line, max_chars) + # If line still exceeds scale_to_size, we were unable to split into a second + # page without truncating. + if len(line) > self.scale_to_size: + raise RuntimeError(f'Line exceeds maximum scale_to_size {self.scale_to_size}' + ' and could not be split.') if self.max_lines is not None: if self._linecount >= self.max_lines: @@ -87,6 +110,36 @@ class LinePaginator(Paginator): self._current_page.append('') self._count += 1 + if remaining_words: + self.add_line(remaining_words) + + def _split_remaining_words(self, line: str, max_chars: int) -> t.Tuple[str, t.Optional[str]]: + """Internal: split a line into two strings; one that fits within *max_chars* characters + (reduced_words) and another for the remaining (remaining_words), rounding down to the + nearest word. + + Return a tuple in the format (reduced_words, remaining_words). + """ + reduced_words = [] + # "(Continued)" is used on a line by itself to indicate the continuation of last page + remaining_words = ["(Continued)\n", "---------------\n"] + reduced_char_count = 0 + is_full = False + + for word in line.split(" "): + if not is_full: + if len(word) + reduced_char_count <= max_chars: + reduced_words.append(word) + reduced_char_count += len(word) + else: + is_full = True + remaining_words.append(word) + else: + remaining_words.append(word) + + return " ".join(reduced_words), " ".join(remaining_words) if len(remaining_words) > 2 \ + else None + @classmethod async def paginate( cls, @@ -97,6 +150,7 @@ class LinePaginator(Paginator): suffix: str = "", max_lines: t.Optional[int] = None, max_size: int = 500, + scale_to_size: int = 2000, empty: bool = True, restrict_to_user: User = None, timeout: int = 300, @@ -147,7 +201,7 @@ class LinePaginator(Paginator): if not lines: if exception_on_empty_embed: - log.exception(f"Pagination asked for empty lines iterable") + log.exception("Pagination asked for empty lines iterable") raise EmptyPaginatorEmbed("No lines to paginate") log.debug("No lines to add to paginator, adding '(nothing to display)' message") @@ -357,7 +411,7 @@ class ImagePaginator(Paginator): if not pages: if exception_on_empty_embed: - log.exception(f"Pagination asked for empty image list") + log.exception("Pagination asked for empty image list") raise EmptyPaginatorEmbed("No images to paginate") log.debug("No images to add to paginator, adding '(no images to display)' message") diff --git a/tests/bot/test_pagination.py b/tests/bot/test_pagination.py index 0a734b505..f2e2c27ce 100644 --- a/tests/bot/test_pagination.py +++ b/tests/bot/test_pagination.py @@ -8,17 +8,44 @@ class LinePaginatorTests(TestCase): def setUp(self): """Create a paginator for the test method.""" - self.paginator = pagination.LinePaginator(prefix='', suffix='', max_size=30) - - def test_add_line_raises_on_too_long_lines(self): - """`add_line` should raise a `RuntimeError` for too long lines.""" - message = f"Line exceeds maximum page size {self.paginator.max_size - 2}" - with self.assertRaises(RuntimeError, msg=message): - self.paginator.add_line('x' * self.paginator.max_size) + self.paginator = pagination.LinePaginator(prefix='', suffix='', max_size=30, + scale_to_size=50) def test_add_line_works_on_small_lines(self): """`add_line` should allow small lines to be added.""" self.paginator.add_line('x' * (self.paginator.max_size - 3)) + # Note that the page isn't added to _pages until it's full. + self.assertEqual(len(self.paginator._pages), 0) + + def test_add_line_works_on_long_lines(self): + """`add_line` should scale long lines up to `scale_to_size`.""" + self.paginator.add_line('x' * self.paginator.scale_to_size) + self.assertEqual(len(self.paginator._pages), 1) + + # Any additional lines should start a new page after `max_size` is exceeded. + self.paginator.add_line('x') + self.assertEqual(len(self.paginator._pages), 2) + + def test_add_line_continuation(self): + """When `scale_to_size` is exceeded, remaining words should be split onto the next page.""" + self.paginator.add_line('zyz ' * (self.paginator.scale_to_size//4 + 1)) + self.assertEqual(len(self.paginator._pages), 2) + + def test_add_line_no_continuation(self): + """If adding a new line to an existing page would exceed `max_size`, it should start a new + page rather than using continuation. + """ + self.paginator.add_line('z' * (self.paginator.max_size - 3)) + self.paginator.add_line('z') + self.assertEqual(len(self.paginator._pages), 1) + + def test_add_line_raises_on_very_long_words(self): + """`add_line` should raise if a single long word is added that exceeds `scale_to_size`. + + Note: truncation is also a potential option, but this should not occur from normal usage. + """ + with self.assertRaises(RuntimeError): + self.paginator.add_line('x' * (self.paginator.scale_to_size + 1)) class ImagePaginatorTests(TestCase): -- cgit v1.2.3 From 78782868040d1b2ca0b655efc4123b3d9b6bfda3 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 10:23:45 +0300 Subject: Jam Tests: Created base test layout --- tests/bot/cogs/test_jams.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 tests/bot/cogs/test_jams.py (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py new file mode 100644 index 000000000..33dee593e --- /dev/null +++ b/tests/bot/cogs/test_jams.py @@ -0,0 +1,14 @@ +import unittest + +from bot.constants import Roles +from tests.helpers import MockBot, MockContext, MockMember, MockRole + + +class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): + """Tests for `createteam` command.""" + + def setUp(self): + self.bot = MockBot() + self.admin_role = MockRole(name="Admins", id=Roles.admins) + self.command_user = MockMember([self.admin_role]) + self.context = MockContext(bot=self.bot, author=self.command_user) -- cgit v1.2.3 From 6242fbdce8935c681fa575b1c208642fe9d2635b Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 10:38:41 +0300 Subject: Jam Tests: Created tests for case when too small amount of members given --- tests/bot/cogs/test_jams.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 33dee593e..3e71370c2 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -1,5 +1,7 @@ import unittest +from unittest.mock import patch +from bot.cogs.jams import CodeJams from bot.constants import Roles from tests.helpers import MockBot, MockContext, MockMember, MockRole @@ -11,4 +13,18 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.bot = MockBot() self.admin_role = MockRole(name="Admins", id=Roles.admins) self.command_user = MockMember([self.admin_role]) - self.context = MockContext(bot=self.bot, author=self.command_user) + self.ctx = MockContext(bot=self.bot, author=self.command_user) + self.cog = CodeJams(self.bot) + + @patch("bot.cogs.jams.utils") + async def test_too_small_amount_of_team_members_passed(self, utils_mock): + """Should `ctx.send` and exit early when too small amount of members.""" + for case in (1, 2): + with self.subTest(amount_of_members=case): + self.ctx.reset_mock() + utils_mock.reset_mock() + await self.cog.createteam( + self.cog, self.ctx, team_name="foo", members=(MockMember() for _ in range(case)) + ) + self.ctx.send.assert_awaited_once() + utils_mock.get.assert_not_called() -- cgit v1.2.3 From a9122b781191f93f5dd375b5c1d9e7744943b464 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 10:46:08 +0300 Subject: Jam Tests: Created tests for removing duplicate team members --- tests/bot/cogs/test_jams.py | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 3e71370c2..1cface1c1 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -28,3 +28,12 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): ) self.ctx.send.assert_awaited_once() utils_mock.get.assert_not_called() + + @patch("bot.cogs.jams.utils") + async def test_duplicate_members_provided(self, utils_mock): + """Should `ctx.send` and exit early because duplicate members provided and total there is only 1 member.""" + self.ctx.reset_mock() + member = MockMember() + await self.cog.createteam(self.cog, self.ctx, "foo", (member for _ in range(5))) + self.ctx.send.assert_awaited_once() + utils_mock.get.assert_not_called() -- cgit v1.2.3 From ebaac5988d7ff1558595008540eab5368312d170 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 17:57:26 +0300 Subject: Jam Tests: Created test for category creating when not exist --- tests/bot/cogs/test_jams.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 1cface1c1..2153178c3 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -3,7 +3,7 @@ from unittest.mock import patch from bot.cogs.jams import CodeJams from bot.constants import Roles -from tests.helpers import MockBot, MockContext, MockMember, MockRole +from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): @@ -13,7 +13,8 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.bot = MockBot() self.admin_role = MockRole(name="Admins", id=Roles.admins) self.command_user = MockMember([self.admin_role]) - self.ctx = MockContext(bot=self.bot, author=self.command_user) + self.guild = MockGuild([self.admin_role]) + self.ctx = MockContext(bot=self.bot, author=self.command_user, guild=self.guild) self.cog = CodeJams(self.bot) @patch("bot.cogs.jams.utils") @@ -37,3 +38,14 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): await self.cog.createteam(self.cog, self.ctx, "foo", (member for _ in range(5))) self.ctx.send.assert_awaited_once() utils_mock.get.assert_not_called() + + @patch("bot.cogs.jams.utils") + async def test_category_dont_exist(self, utils_mock): + """Should create code jam category.""" + utils_mock.get.return_value = None + await self.cog.createteam(self.cog, self.ctx, "foo", (MockMember() for _ in range(5))) + self.ctx.guild.create_category_channel.assert_awaited_once() + category_overwrites = self.ctx.guild.create_category_channel.call_args[1]["overwrites"] + + self.assertFalse(category_overwrites[self.ctx.guild.default_role].read_messages) + self.assertTrue(category_overwrites[self.ctx.guild.me].read_messages) -- cgit v1.2.3 From 14d4eda8b1e7839b286402091ac060d3c869f447 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 17:58:38 +0300 Subject: Jam Tests: Added utils.get assert to category creating test --- tests/bot/cogs/test_jams.py | 1 + 1 file changed, 1 insertion(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 2153178c3..f5f87761b 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -44,6 +44,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should create code jam category.""" utils_mock.get.return_value = None await self.cog.createteam(self.cog, self.ctx, "foo", (MockMember() for _ in range(5))) + utils_mock.get.assert_called_once() self.ctx.guild.create_category_channel.assert_awaited_once() category_overwrites = self.ctx.guild.create_category_channel.call_args[1]["overwrites"] -- cgit v1.2.3 From 464c4bbb53101d4456314bf7a40243337525d514 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 18:03:17 +0300 Subject: Jam Tests: Created test that make sure when category exist, don't create --- tests/bot/cogs/test_jams.py | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index f5f87761b..1ce71a942 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -50,3 +50,11 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.assertFalse(category_overwrites[self.ctx.guild.default_role].read_messages) self.assertTrue(category_overwrites[self.ctx.guild.me].read_messages) + + @patch("bot.cogs.jams.utils") + async def test_category_channel_exist(self, utils_mock): + """Should not try to create category channel.""" + utils_mock.return_value = "foo" + await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) + utils_mock.get.assert_called_once() + self.ctx.guild.create_category_channel.assert_not_awaited() -- cgit v1.2.3 From a63545510f392cf3e36e310b68792177a178b769 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 18:08:29 +0300 Subject: Jam Tests: Created test for creating text channel for team --- tests/bot/cogs/test_jams.py | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 1ce71a942..9d26628ff 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -58,3 +58,8 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) utils_mock.get.assert_called_once() self.ctx.guild.create_category_channel.assert_not_awaited() + + async def test_team_text_channel_creation(self): + """Should create text channel for team.""" + await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) + self.ctx.guild.create_text_channel.assert_awaited_once() -- cgit v1.2.3 From 3df28c1b2a64bee3a52442fe42decaa960c45fde Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 18:18:09 +0300 Subject: Jam Tests: Created test for channel overwrites --- tests/bot/cogs/test_jams.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 9d26628ff..d21c5ea29 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -63,3 +63,27 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should create text channel for team.""" await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) self.ctx.guild.create_text_channel.assert_awaited_once() + + async def test_channel_overwrites(self): + """Should have correct permission overwrites for users and roles.""" + leader = MockMember() + members = [leader] + [MockMember() for _ in range(4)] + await self.cog.createteam(self.cog, self.ctx, "foo", members) + overwrites = self.ctx.guild.create_text_channel.call_args[1]["overwrites"] + + # Leader permission overwrites + self.assertTrue(overwrites[leader].manage_messages) + self.assertTrue(overwrites[leader].read_messages) + self.assertTrue(overwrites[leader].manage_webhooks) + self.assertTrue(overwrites[leader].connect) + + # Other members permission overwrites + for member in members[1:]: + self.assertTrue(overwrites[member].read_messages) + self.assertTrue(overwrites[member].connect) + + # Everyone and verified role overwrite + self.assertFalse(overwrites[self.ctx.guild.default_role].read_messages) + self.assertFalse(overwrites[self.ctx.guild.default_role].connect) + self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].read_messages) + self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].connect) -- cgit v1.2.3 From 6476d3ba6dfc28441d097aaa15a7c9e13f53f646 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 18:22:02 +0300 Subject: Jam Tests: Make text channel creation test more specific --- tests/bot/cogs/test_jams.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index d21c5ea29..94c48b995 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -59,11 +59,18 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): utils_mock.get.assert_called_once() self.ctx.guild.create_category_channel.assert_not_awaited() - async def test_team_text_channel_creation(self): + @patch("bot.cogs.jams.utils") + async def test_team_text_channel_creation(self, utils_mock): """Should create text channel for team.""" + utils_mock.get.return_value = "foo" await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) + # Make sure that we awaited function before getting call arguments self.ctx.guild.create_text_channel.assert_awaited_once() + # All other arguments is possible to get somewhere else except this + overwrites = self.ctx.guild.create_text_channel.call_args[1]["overwrites"] + self.ctx.guild.create_text_channel.assert_awaited_once_with("bar", overwrites=overwrites, category="foo") + async def test_channel_overwrites(self): """Should have correct permission overwrites for users and roles.""" leader = MockMember() -- cgit v1.2.3 From b1359f0ed37cdbbb6bae9dbbe92e3bf0db660636 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 18:26:41 +0300 Subject: Jam Tests: Create test for team voice channel creating --- tests/bot/cogs/test_jams.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 94c48b995..2e1419f8e 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -94,3 +94,15 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.assertFalse(overwrites[self.ctx.guild.default_role].connect) self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].read_messages) self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].connect) + + @patch("bot.cogs.jams.utils") + async def test_team_voice_channel_creation(self, utils_mock): + """Should create new voice channel for team.""" + utils_mock.get.return_value = "foo" + await self.cog.createteam(self.cog, self.ctx, "my-team", (MockMember() for _ in range(5))) + # Make sure that we awaited function before getting call arguments + self.ctx.guild.create_voice_channel.assert_awaited_once() + + # All other arguments is possible to get somewhere else except this + overwrites = self.ctx.guild.create_voice_channel.call_args[1]["overwrites"] + self.ctx.guild.create_voice_channel.assert_awaited_once_with("My Team", overwrites=overwrites, category="foo") -- cgit v1.2.3 From b5b05adc41e55dd58810608f4ac7ade6281cdf84 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 18:37:27 +0300 Subject: Jam Tests: Create test for team jam roles adding --- tests/bot/cogs/test_jams.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 2e1419f8e..16caa98c6 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -106,3 +106,17 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): # All other arguments is possible to get somewhere else except this overwrites = self.ctx.guild.create_voice_channel.call_args[1]["overwrites"] self.ctx.guild.create_voice_channel.assert_awaited_once_with("My Team", overwrites=overwrites, category="foo") + + async def test_jam_roles_adding(self): + """Should add team leader role to leader and jam role to every team member.""" + leader_role = MockRole(name="Team Leader") + jam_role = MockRole(name="Jammer") + self.ctx.guild.get_role.side_effect = [MockRole(), leader_role, jam_role] + + leader = MockMember() + members = [leader] + [MockMember() for _ in range(4)] + await self.cog.createteam(self.cog, self.ctx, "foo", members) + + leader.add_roles.assert_any_await(leader_role) + for member in members: + member.add_roles.assert_any_await(jam_role) -- cgit v1.2.3 From 76ad4d141027f6351e2feedc466c8acc805f671d Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 18:39:13 +0300 Subject: Jam Tests: Create test for successful `ctx.send` calling --- tests/bot/cogs/test_jams.py | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 16caa98c6..7db66ff11 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -120,3 +120,9 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): leader.add_roles.assert_any_await(leader_role) for member in members: member.add_roles.assert_any_await(jam_role) + + async def test_result_sending(self): + """Should call `ctx.send` when everything go right.""" + self.ctx.reset_mock() + await self.cog.createteam(self.cog, self.ctx, "foo", (MockMember() for _ in range(5))) + self.ctx.send.assert_awaited_once() -- cgit v1.2.3 From bbe4f137bd583d66a6bcb03102327bc6c586af86 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 23 May 2020 18:42:03 +0300 Subject: Jam Tests: Create test for `setup` function --- tests/bot/cogs/test_jams.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 7db66ff11..2c5cef835 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -1,7 +1,7 @@ import unittest from unittest.mock import patch -from bot.cogs.jams import CodeJams +from bot.cogs.jams import CodeJams, setup from bot.constants import Roles from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole @@ -126,3 +126,13 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.ctx.reset_mock() await self.cog.createteam(self.cog, self.ctx, "foo", (MockMember() for _ in range(5))) self.ctx.send.assert_awaited_once() + + +class CodeJamSetup(unittest.TestCase): + """Test for `setup` function of `CodeJam` cog.""" + + def test_setup(self): + """Should call `bot.add_cog`.""" + bot = MockBot() + setup(bot) + bot.add_cog.assert_called_once() -- cgit v1.2.3 From 5ca860fb3b2bcb77ab8574d83e8159df471f0faf Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 08:54:07 +0300 Subject: Jam Tests: Fix `test_result_sending` docstring Co-authored-by: Mark --- tests/bot/cogs/test_jams.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 2c5cef835..51720d957 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -122,7 +122,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): member.add_roles.assert_any_await(jam_role) async def test_result_sending(self): - """Should call `ctx.send` when everything go right.""" + """Should call `ctx.send` when everything goes right.""" self.ctx.reset_mock() await self.cog.createteam(self.cog, self.ctx, "foo", (MockMember() for _ in range(5))) self.ctx.send.assert_awaited_once() -- cgit v1.2.3 From 28f33584b65b1f9d7e7254b4822d8896c7f19284 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 09:26:43 +0300 Subject: Jam Tests: Use class member of patch instead decorator on most of tests --- tests/bot/cogs/test_jams.py | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 51720d957..bf542458b 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -16,53 +16,52 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.guild = MockGuild([self.admin_role]) self.ctx = MockContext(bot=self.bot, author=self.command_user, guild=self.guild) self.cog = CodeJams(self.bot) + self.utils_mock = patch("bot.cogs.jams.utils").start() - @patch("bot.cogs.jams.utils") - async def test_too_small_amount_of_team_members_passed(self, utils_mock): + def tearDown(self): + self.utils_mock.stop() + + async def test_too_small_amount_of_team_members_passed(self): """Should `ctx.send` and exit early when too small amount of members.""" for case in (1, 2): with self.subTest(amount_of_members=case): self.ctx.reset_mock() - utils_mock.reset_mock() + self.utils_mock.reset_mock() await self.cog.createteam( self.cog, self.ctx, team_name="foo", members=(MockMember() for _ in range(case)) ) self.ctx.send.assert_awaited_once() - utils_mock.get.assert_not_called() + self.utils_mock.get.assert_not_called() - @patch("bot.cogs.jams.utils") - async def test_duplicate_members_provided(self, utils_mock): + async def test_duplicate_members_provided(self): """Should `ctx.send` and exit early because duplicate members provided and total there is only 1 member.""" self.ctx.reset_mock() member = MockMember() await self.cog.createteam(self.cog, self.ctx, "foo", (member for _ in range(5))) self.ctx.send.assert_awaited_once() - utils_mock.get.assert_not_called() + self.utils_mock.get.assert_not_called() - @patch("bot.cogs.jams.utils") - async def test_category_dont_exist(self, utils_mock): + async def test_category_dont_exist(self): """Should create code jam category.""" - utils_mock.get.return_value = None + self.utils_mock.get.return_value = None await self.cog.createteam(self.cog, self.ctx, "foo", (MockMember() for _ in range(5))) - utils_mock.get.assert_called_once() + self.utils_mock.get.assert_called_once() self.ctx.guild.create_category_channel.assert_awaited_once() category_overwrites = self.ctx.guild.create_category_channel.call_args[1]["overwrites"] self.assertFalse(category_overwrites[self.ctx.guild.default_role].read_messages) self.assertTrue(category_overwrites[self.ctx.guild.me].read_messages) - @patch("bot.cogs.jams.utils") - async def test_category_channel_exist(self, utils_mock): + async def test_category_channel_exist(self): """Should not try to create category channel.""" - utils_mock.return_value = "foo" + self.utils_mock.return_value = "foo" await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) - utils_mock.get.assert_called_once() + self.utils_mock.get.assert_called_once() self.ctx.guild.create_category_channel.assert_not_awaited() - @patch("bot.cogs.jams.utils") - async def test_team_text_channel_creation(self, utils_mock): + async def test_team_text_channel_creation(self): """Should create text channel for team.""" - utils_mock.get.return_value = "foo" + self.utils_mock.get.return_value = "foo" await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) # Make sure that we awaited function before getting call arguments self.ctx.guild.create_text_channel.assert_awaited_once() @@ -95,10 +94,9 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].read_messages) self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].connect) - @patch("bot.cogs.jams.utils") - async def test_team_voice_channel_creation(self, utils_mock): + async def test_team_voice_channel_creation(self): """Should create new voice channel for team.""" - utils_mock.get.return_value = "foo" + self.utils_mock.get.return_value = "foo" await self.cog.createteam(self.cog, self.ctx, "my-team", (MockMember() for _ in range(5))) # Make sure that we awaited function before getting call arguments self.ctx.guild.create_voice_channel.assert_awaited_once() -- cgit v1.2.3 From 1c860606a122ff1378cb55e228312acb2bb2d49e Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:16:50 +0300 Subject: Jam Tests: Make early exiting test more secure --- tests/bot/cogs/test_jams.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index bf542458b..98fa12f66 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -1,5 +1,5 @@ import unittest -from unittest.mock import patch +from unittest.mock import AsyncMock, patch from bot.cogs.jams import CodeJams, setup from bot.constants import Roles @@ -25,13 +25,17 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should `ctx.send` and exit early when too small amount of members.""" for case in (1, 2): with self.subTest(amount_of_members=case): + self.cog.create_channels = AsyncMock() + self.cog.add_roles = AsyncMock() + self.ctx.reset_mock() self.utils_mock.reset_mock() await self.cog.createteam( self.cog, self.ctx, team_name="foo", members=(MockMember() for _ in range(case)) ) self.ctx.send.assert_awaited_once() - self.utils_mock.get.assert_not_called() + self.cog.create_channels.assert_not_awaited() + self.cog.add_roles.assert_not_awaited() async def test_duplicate_members_provided(self): """Should `ctx.send` and exit early because duplicate members provided and total there is only 1 member.""" -- cgit v1.2.3 From fd05997c1aa9054024ad62dc0cbf19c1a296f4b7 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:22:52 +0300 Subject: Jam Tests: Add more assertions to result message sending test --- tests/bot/cogs/test_jams.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 98fa12f66..4307d7deb 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -126,8 +126,14 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_result_sending(self): """Should call `ctx.send` when everything goes right.""" self.ctx.reset_mock() - await self.cog.createteam(self.cog, self.ctx, "foo", (MockMember() for _ in range(5))) + members = [MockMember() for _ in range(5)] + await self.cog.createteam(self.cog, self.ctx, "foo", members) self.ctx.send.assert_awaited_once() + sent_string = self.ctx.send.call_args[0][0] + + self.assertIn(str(self.ctx.guild.create_text_channel.return_value.mention), sent_string) + self.assertIn(members[0].mention, sent_string) + self.assertIn(" ".join(member.mention for member in members[1:]), sent_string) class CodeJamSetup(unittest.TestCase): -- cgit v1.2.3 From fa4783c5e15709625e21d6a1aa766664eb2423e2 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:24:27 +0300 Subject: Jam Tests: Apply recent changes to overwrites test --- tests/bot/cogs/test_jams.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 4307d7deb..1cbff2674 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -78,8 +78,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should have correct permission overwrites for users and roles.""" leader = MockMember() members = [leader] + [MockMember() for _ in range(4)] - await self.cog.createteam(self.cog, self.ctx, "foo", members) - overwrites = self.ctx.guild.create_text_channel.call_args[1]["overwrites"] + overwrites = self.cog.get_overwrites(members, self.ctx) # Leader permission overwrites self.assertTrue(overwrites[leader].manage_messages) -- cgit v1.2.3 From 0d2b61fd72f7b44d0534901c8f2e6ee3ccaad3f7 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:36:11 +0300 Subject: Jam Tests: Merge text and voice channel creation tests --- tests/bot/cogs/test_jams.py | 45 ++++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 21 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 1cbff2674..54f906ed9 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -1,9 +1,9 @@ import unittest -from unittest.mock import AsyncMock, patch +from unittest.mock import AsyncMock, MagicMock, patch from bot.cogs.jams import CodeJams, setup from bot.constants import Roles -from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole +from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole, MockTextChannel class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): @@ -63,17 +63,6 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.utils_mock.get.assert_called_once() self.ctx.guild.create_category_channel.assert_not_awaited() - async def test_team_text_channel_creation(self): - """Should create text channel for team.""" - self.utils_mock.get.return_value = "foo" - await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) - # Make sure that we awaited function before getting call arguments - self.ctx.guild.create_text_channel.assert_awaited_once() - - # All other arguments is possible to get somewhere else except this - overwrites = self.ctx.guild.create_text_channel.call_args[1]["overwrites"] - self.ctx.guild.create_text_channel.assert_awaited_once_with("bar", overwrites=overwrites, category="foo") - async def test_channel_overwrites(self): """Should have correct permission overwrites for users and roles.""" leader = MockMember() @@ -97,16 +86,30 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].read_messages) self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].connect) - async def test_team_voice_channel_creation(self): - """Should create new voice channel for team.""" + async def test_team_channels_creation(self): + """Should create new voice and text channel for team.""" self.utils_mock.get.return_value = "foo" - await self.cog.createteam(self.cog, self.ctx, "my-team", (MockMember() for _ in range(5))) - # Make sure that we awaited function before getting call arguments - self.ctx.guild.create_voice_channel.assert_awaited_once() + members = [MockMember() for _ in range(5)] - # All other arguments is possible to get somewhere else except this - overwrites = self.ctx.guild.create_voice_channel.call_args[1]["overwrites"] - self.ctx.guild.create_voice_channel.assert_awaited_once_with("My Team", overwrites=overwrites, category="foo") + self.cog.get_overwrites = MagicMock() + self.cog.get_category = AsyncMock() + self.ctx.guild.create_text_channel.return_value = MockTextChannel(mention="foobar-channel") + actual = await self.cog.create_channels(self.ctx, "my-team", members) + + self.assertEqual("foobar-channel", actual) + self.cog.get_overwrites.assert_called_once_with(members, self.ctx) + self.cog.get_category.assert_awaited_once_with(self.ctx) + + self.ctx.guild.create_text_channel.assert_awaited_once_with( + "my-team", + overwrites=self.cog.get_overwrites.return_value, + category=self.cog.get_category.return_value + ) + self.ctx.guild.create_voice_channel.assert_awaited_once_with( + "My Team", + overwrites=self.cog.get_overwrites.return_value, + category=self.cog.get_category.return_value + ) async def test_jam_roles_adding(self): """Should add team leader role to leader and jam role to every team member.""" -- cgit v1.2.3 From 4af2be7310141ab3ddc34a2184366c0d8212cdd5 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:39:46 +0300 Subject: Jam Tests: Simplify and update `test_category_channel_exist` --- tests/bot/cogs/test_jams.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 54f906ed9..ae3e35dbb 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -58,9 +58,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_category_channel_exist(self): """Should not try to create category channel.""" - self.utils_mock.return_value = "foo" - await self.cog.createteam(self.cog, self.ctx, "bar", (MockMember() for _ in range(5))) - self.utils_mock.get.assert_called_once() + await self.cog.get_category(self.ctx) self.ctx.guild.create_category_channel.assert_not_awaited() async def test_channel_overwrites(self): -- cgit v1.2.3 From ea91aefe55bf52fca6714897347bb24d4a4efb5b Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:41:25 +0300 Subject: Jam Tests: Apply recent changes to `test_category_dont_exist` --- tests/bot/cogs/test_jams.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index ae3e35dbb..ecd06179f 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -48,8 +48,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_category_dont_exist(self): """Should create code jam category.""" self.utils_mock.get.return_value = None - await self.cog.createteam(self.cog, self.ctx, "foo", (MockMember() for _ in range(5))) - self.utils_mock.get.assert_called_once() + await self.cog.get_category(self.ctx) self.ctx.guild.create_category_channel.assert_awaited_once() category_overwrites = self.ctx.guild.create_category_channel.call_args[1]["overwrites"] -- cgit v1.2.3 From 6e070a43f616f898e328bfc4581ed48551e73b12 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:47:13 +0300 Subject: Jam Tests: Implement default arguments To avoid repeating same arguments, added default arguments that is unpacked on function call. --- tests/bot/cogs/test_jams.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index ecd06179f..94be8dd03 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -17,6 +17,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.ctx = MockContext(bot=self.bot, author=self.command_user, guild=self.guild) self.cog = CodeJams(self.bot) self.utils_mock = patch("bot.cogs.jams.utils").start() + self.default_args = [self.cog, self.ctx, "foo"] def tearDown(self): self.utils_mock.stop() @@ -30,9 +31,8 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.ctx.reset_mock() self.utils_mock.reset_mock() - await self.cog.createteam( - self.cog, self.ctx, team_name="foo", members=(MockMember() for _ in range(case)) - ) + await self.cog.createteam(*self.default_args, (MockMember() for _ in range(case))) + self.ctx.send.assert_awaited_once() self.cog.create_channels.assert_not_awaited() self.cog.add_roles.assert_not_awaited() @@ -41,7 +41,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should `ctx.send` and exit early because duplicate members provided and total there is only 1 member.""" self.ctx.reset_mock() member = MockMember() - await self.cog.createteam(self.cog, self.ctx, "foo", (member for _ in range(5))) + await self.cog.createteam(*self.default_args, (member for _ in range(5))) self.ctx.send.assert_awaited_once() self.utils_mock.get.assert_not_called() -- cgit v1.2.3 From b129658bf260d458d5fad5925e945c78f881388a Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:49:00 +0300 Subject: Jam Tests: Remove unnecessary `Context` mock resets --- tests/bot/cogs/test_jams.py | 2 -- 1 file changed, 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 94be8dd03..0f8ba3574 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -39,7 +39,6 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_duplicate_members_provided(self): """Should `ctx.send` and exit early because duplicate members provided and total there is only 1 member.""" - self.ctx.reset_mock() member = MockMember() await self.cog.createteam(*self.default_args, (member for _ in range(5))) self.ctx.send.assert_awaited_once() @@ -124,7 +123,6 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_result_sending(self): """Should call `ctx.send` when everything goes right.""" - self.ctx.reset_mock() members = [MockMember() for _ in range(5)] await self.cog.createteam(self.cog, self.ctx, "foo", members) self.ctx.send.assert_awaited_once() -- cgit v1.2.3 From 0481bcc1d99dd9d7fe9d41276599437b11670b27 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Thu, 11 Jun 2020 11:50:40 +0300 Subject: Jam Tests: Apply recent command splitting to `test_jam_roles_adding` --- tests/bot/cogs/test_jams.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 0f8ba3574..54fe0b5f2 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -111,11 +111,11 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should add team leader role to leader and jam role to every team member.""" leader_role = MockRole(name="Team Leader") jam_role = MockRole(name="Jammer") - self.ctx.guild.get_role.side_effect = [MockRole(), leader_role, jam_role] + self.ctx.guild.get_role.side_effect = [leader_role, jam_role] leader = MockMember() members = [leader] + [MockMember() for _ in range(4)] - await self.cog.createteam(self.cog, self.ctx, "foo", members) + await self.cog.add_roles(self.ctx, members) leader.add_roles.assert_any_await(leader_role) for member in members: -- cgit v1.2.3 From 5c70a7dad3ee59e865df08affe7905a843a823ce Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 12 Jun 2020 22:05:15 +0200 Subject: Incidents tests: create new test module --- tests/bot/cogs/moderation/test_incidents.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 tests/bot/cogs/moderation/test_incidents.py (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py new file mode 100644 index 000000000..e69de29bb -- cgit v1.2.3 From ae5028d5966ba126f902783db8ad685646f45f37 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 12 Jun 2020 23:14:41 +0200 Subject: Incidents tests: write tests for module-level helpers --- tests/bot/cogs/moderation/test_incidents.py | 135 ++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index e69de29bb..4c1f9bc07 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -0,0 +1,135 @@ +import enum +import unittest +from unittest.mock import AsyncMock, MagicMock, call, patch + +import discord + +from bot.cogs.moderation import incidents + + +@patch("bot.constants.Channels.incidents", 123) +class TestIsIncident(unittest.TestCase): + """ + Collection of tests for the `is_incident` helper function. + + In `setUp`, we will create a mock message which should qualify as an incident. Each + test case will then mutate this instance to make it **not** qualify, in various ways. + + Notice that we patch the #incidents channel id globally for this class. + """ + + def setUp(self) -> None: + """Prepare a mock message which should qualify as an incident.""" + self.incident = MagicMock( + discord.Message, + channel=MagicMock(discord.TextChannel, id=123), + content="this is an incident", + author=MagicMock(discord.User, bot=False), + pinned=False, + ) + + def test_is_incident_true(self): + """Message qualifies as an incident if unchanged.""" + self.assertTrue(incidents.is_incident(self.incident)) + + def check_false(self): + """Assert that `self.incident` does **not** qualify as an incident.""" + self.assertFalse(incidents.is_incident(self.incident)) + + def test_is_incident_false_channel(self): + """Message doesn't qualify if sent outside of #incidents.""" + self.incident.channel = MagicMock(discord.TextChannel, id=456) + self.check_false() + + def test_is_incident_false_content(self): + """Message doesn't qualify if content begins with hash symbol.""" + self.incident.content = "# this is a comment message" + self.check_false() + + def test_is_incident_false_author(self): + """Message doesn't qualify if author is a bot.""" + self.incident.author = MagicMock(discord.User, bot=True) + self.check_false() + + def test_is_incident_false_pinned(self): + """Message doesn't qualify if it is pinned.""" + self.incident.pinned = True + self.check_false() + + +class TestOwnReactions(unittest.TestCase): + """Assertions for the `own_reactions` function.""" + + def test_own_reactions(self): + """Only bot's own emoji are extracted from the input incident.""" + reactions = ( + MagicMock(discord.Reaction, emoji="A", me=True), + MagicMock(discord.Reaction, emoji="B", me=True), + MagicMock(discord.Reaction, emoji="C", me=False), + ) + message = MagicMock(discord.Message, reactions=reactions) + self.assertSetEqual(incidents.own_reactions(message), {"A", "B"}) + + +@patch("bot.cogs.moderation.incidents.ALLOWED_EMOJI", {"A", "B"}) +class TestHasSignals(unittest.TestCase): + """ + Assertions for the `has_signals` function. + + We patch `ALLOWED_EMOJI` globally. Each test function then patches `own_reactions` + as appropriate. + """ + + def test_has_signals_true(self): + """True when `own_reactions` returns all emoji in `ALLOWED_EMOJI`.""" + message = MagicMock(discord.Message) + own_reactions = MagicMock(return_value={"A", "B"}) + + with patch("bot.cogs.moderation.incidents.own_reactions", own_reactions): + self.assertTrue(incidents.has_signals(message)) + + def test_has_signals_false(self): + """False when `own_reactions` does not return all emoji in `ALLOWED_EMOJI`.""" + message = MagicMock(discord.Message) + own_reactions = MagicMock(return_value={"A", "C"}) + + with patch("bot.cogs.moderation.incidents.own_reactions", own_reactions): + self.assertFalse(incidents.has_signals(message)) + + +class Signal(enum.Enum): + A = "A" + B = "B" + + +@patch("bot.cogs.moderation.incidents.Signal", Signal) +class TestAddSignals(unittest.IsolatedAsyncioTestCase): + """ + Assertions for the `add_signals` coroutine. + + These are all fairly similar and could go into a single test function, but I found the + patching & sub-testing fairly awkward in that case and decided to split them up + to avoid unnecessary syntax noise. + """ + + def setUp(self): + """Prepare a mock incident message for tests to use.""" + self.incident = MagicMock(discord.Message, add_reaction=AsyncMock()) + + @patch("bot.cogs.moderation.incidents.own_reactions", MagicMock(return_value=set())) + async def test_add_signals_missing(self): + """All emoji are added when none are present.""" + await incidents.add_signals(self.incident) + self.incident.add_reaction.assert_has_calls([call("A"), call("B")]) + + @patch("bot.cogs.moderation.incidents.own_reactions", MagicMock(return_value={"A"})) + async def test_add_signals_partial(self): + """Only missing emoji are added when some are present.""" + await incidents.add_signals(self.incident) + self.incident.add_reaction.assert_has_calls([call("B")]) + + @patch("bot.cogs.moderation.incidents.own_reactions", MagicMock(return_value={"A", "B"})) + async def test_add_signals_present(self): + """No emoji are added when all are present.""" + await incidents.add_signals(self.incident) + self.incident.add_reaction.assert_not_called() -- cgit v1.2.3 From 9dbfe7da4cbc4d1820507e25ce56929b7fb55327 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 13 Jun 2020 08:26:19 +0300 Subject: Jam Tests: Update `Context` to `Guild` for tests too --- tests/bot/cogs/test_jams.py | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 54fe0b5f2..17b86601f 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -47,23 +47,23 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_category_dont_exist(self): """Should create code jam category.""" self.utils_mock.get.return_value = None - await self.cog.get_category(self.ctx) - self.ctx.guild.create_category_channel.assert_awaited_once() - category_overwrites = self.ctx.guild.create_category_channel.call_args[1]["overwrites"] + await self.cog.get_category(self.guild) + self.guild.create_category_channel.assert_awaited_once() + category_overwrites = self.guild.create_category_channel.call_args[1]["overwrites"] - self.assertFalse(category_overwrites[self.ctx.guild.default_role].read_messages) - self.assertTrue(category_overwrites[self.ctx.guild.me].read_messages) + self.assertFalse(category_overwrites[self.guild.default_role].read_messages) + self.assertTrue(category_overwrites[self.guild.me].read_messages) async def test_category_channel_exist(self): """Should not try to create category channel.""" - await self.cog.get_category(self.ctx) - self.ctx.guild.create_category_channel.assert_not_awaited() + await self.cog.get_category(self.guild) + self.guild.create_category_channel.assert_not_awaited() async def test_channel_overwrites(self): """Should have correct permission overwrites for users and roles.""" leader = MockMember() members = [leader] + [MockMember() for _ in range(4)] - overwrites = self.cog.get_overwrites(members, self.ctx) + overwrites = self.cog.get_overwrites(members, self.guild) # Leader permission overwrites self.assertTrue(overwrites[leader].manage_messages) @@ -77,10 +77,10 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.assertTrue(overwrites[member].connect) # Everyone and verified role overwrite - self.assertFalse(overwrites[self.ctx.guild.default_role].read_messages) - self.assertFalse(overwrites[self.ctx.guild.default_role].connect) - self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].read_messages) - self.assertFalse(overwrites[self.ctx.guild.get_role(Roles.verified)].connect) + self.assertFalse(overwrites[self.guild.default_role].read_messages) + self.assertFalse(overwrites[self.guild.default_role].connect) + self.assertFalse(overwrites[self.guild.get_role(Roles.verified)].read_messages) + self.assertFalse(overwrites[self.guild.get_role(Roles.verified)].connect) async def test_team_channels_creation(self): """Should create new voice and text channel for team.""" @@ -90,18 +90,18 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.cog.get_overwrites = MagicMock() self.cog.get_category = AsyncMock() self.ctx.guild.create_text_channel.return_value = MockTextChannel(mention="foobar-channel") - actual = await self.cog.create_channels(self.ctx, "my-team", members) + actual = await self.cog.create_channels(self.guild, "my-team", members) self.assertEqual("foobar-channel", actual) - self.cog.get_overwrites.assert_called_once_with(members, self.ctx) - self.cog.get_category.assert_awaited_once_with(self.ctx) + self.cog.get_overwrites.assert_called_once_with(members, self.guild) + self.cog.get_category.assert_awaited_once_with(self.guild) - self.ctx.guild.create_text_channel.assert_awaited_once_with( + self.guild.create_text_channel.assert_awaited_once_with( "my-team", overwrites=self.cog.get_overwrites.return_value, category=self.cog.get_category.return_value ) - self.ctx.guild.create_voice_channel.assert_awaited_once_with( + self.guild.create_voice_channel.assert_awaited_once_with( "My Team", overwrites=self.cog.get_overwrites.return_value, category=self.cog.get_category.return_value @@ -111,11 +111,11 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should add team leader role to leader and jam role to every team member.""" leader_role = MockRole(name="Team Leader") jam_role = MockRole(name="Jammer") - self.ctx.guild.get_role.side_effect = [leader_role, jam_role] + self.guild.get_role.side_effect = [leader_role, jam_role] leader = MockMember() members = [leader] + [MockMember() for _ in range(4)] - await self.cog.add_roles(self.ctx, members) + await self.cog.add_roles(self.guild, members) leader.add_roles.assert_any_await(leader_role) for member in members: -- cgit v1.2.3 From 2489b144b5bf131ec8b1b42e2ae1dd249cce4d3f Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 13 Jun 2020 08:35:35 +0300 Subject: Jam Tests: Simplify and make tests more secure --- tests/bot/cogs/test_jams.py | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 17b86601f..2d2eebabf 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -42,7 +42,8 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): member = MockMember() await self.cog.createteam(*self.default_args, (member for _ in range(5))) self.ctx.send.assert_awaited_once() - self.utils_mock.get.assert_not_called() + self.cog.create_channels.assert_now_awaited() + self.cog.add_roles.assert_not_awaited() async def test_category_dont_exist(self): """Should create code jam category.""" @@ -125,12 +126,9 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should call `ctx.send` when everything goes right.""" members = [MockMember() for _ in range(5)] await self.cog.createteam(self.cog, self.ctx, "foo", members) + self.cog.create_channel.assert_awaited_once() + self.cog.add_roles.assert_awaited_once() self.ctx.send.assert_awaited_once() - sent_string = self.ctx.send.call_args[0][0] - - self.assertIn(str(self.ctx.guild.create_text_channel.return_value.mention), sent_string) - self.assertIn(members[0].mention, sent_string) - self.assertIn(" ".join(member.mention for member in members[1:]), sent_string) class CodeJamSetup(unittest.TestCase): -- cgit v1.2.3 From 95ae613173bb87719155a95494fe448a45a2d6bc Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 13 Jun 2020 08:39:35 +0300 Subject: Jam Tests: Fix wrong function name and convert them to mocks --- tests/bot/cogs/test_jams.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 2d2eebabf..a66658134 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -124,9 +124,11 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_result_sending(self): """Should call `ctx.send` when everything goes right.""" + self.cog.create_channels = AsyncMock() + self.cog.add_roles = AsyncMock() members = [MockMember() for _ in range(5)] await self.cog.createteam(self.cog, self.ctx, "foo", members) - self.cog.create_channel.assert_awaited_once() + self.cog.create_channels.assert_awaited_once() self.cog.add_roles.assert_awaited_once() self.ctx.send.assert_awaited_once() -- cgit v1.2.3 From ef67747e59892d1307246bcad4d32e245098ff58 Mon Sep 17 00:00:00 2001 From: ks129 <45097959+ks129@users.noreply.github.com> Date: Sat, 13 Jun 2020 08:44:56 +0300 Subject: Jam Tests: Fix `test_duplicate_member_provided` assertions --- tests/bot/cogs/test_jams.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index a66658134..2f2cb4695 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -39,10 +39,12 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_duplicate_members_provided(self): """Should `ctx.send` and exit early because duplicate members provided and total there is only 1 member.""" + self.cog.create_channels = AsyncMock() + self.cog.add_roles = AsyncMock() member = MockMember() await self.cog.createteam(*self.default_args, (member for _ in range(5))) self.ctx.send.assert_awaited_once() - self.cog.create_channels.assert_now_awaited() + self.cog.create_channels.assert_not_awaited() self.cog.add_roles.assert_not_awaited() async def test_category_dont_exist(self): -- cgit v1.2.3 From e9724dad79e7dab3bb801f50770bb06cf8461019 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 15:12:38 +0200 Subject: Incidents tests: use our own helper mocks No reason to build own MagicMocks as we already have helpers that more accurately mimic the mocked behaviour. --- tests/bot/cogs/moderation/test_incidents.py | 30 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 16 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 4c1f9bc07..d7cc84734 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -1,10 +1,9 @@ import enum import unittest -from unittest.mock import AsyncMock, MagicMock, call, patch - -import discord +from unittest.mock import MagicMock, call, patch from bot.cogs.moderation import incidents +from tests.helpers import MockMessage, MockReaction, MockTextChannel, MockUser @patch("bot.constants.Channels.incidents", 123) @@ -20,11 +19,10 @@ class TestIsIncident(unittest.TestCase): def setUp(self) -> None: """Prepare a mock message which should qualify as an incident.""" - self.incident = MagicMock( - discord.Message, - channel=MagicMock(discord.TextChannel, id=123), + self.incident = MockMessage( + channel=MockTextChannel(id=123), content="this is an incident", - author=MagicMock(discord.User, bot=False), + author=MockUser(bot=False), pinned=False, ) @@ -38,7 +36,7 @@ class TestIsIncident(unittest.TestCase): def test_is_incident_false_channel(self): """Message doesn't qualify if sent outside of #incidents.""" - self.incident.channel = MagicMock(discord.TextChannel, id=456) + self.incident.channel = MockTextChannel(id=456) self.check_false() def test_is_incident_false_content(self): @@ -48,7 +46,7 @@ class TestIsIncident(unittest.TestCase): def test_is_incident_false_author(self): """Message doesn't qualify if author is a bot.""" - self.incident.author = MagicMock(discord.User, bot=True) + self.incident.author = MockUser(bot=True) self.check_false() def test_is_incident_false_pinned(self): @@ -63,11 +61,11 @@ class TestOwnReactions(unittest.TestCase): def test_own_reactions(self): """Only bot's own emoji are extracted from the input incident.""" reactions = ( - MagicMock(discord.Reaction, emoji="A", me=True), - MagicMock(discord.Reaction, emoji="B", me=True), - MagicMock(discord.Reaction, emoji="C", me=False), + MockReaction(emoji="A", me=True), + MockReaction(emoji="B", me=True), + MockReaction(emoji="C", me=False), ) - message = MagicMock(discord.Message, reactions=reactions) + message = MockMessage(reactions=reactions) self.assertSetEqual(incidents.own_reactions(message), {"A", "B"}) @@ -82,7 +80,7 @@ class TestHasSignals(unittest.TestCase): def test_has_signals_true(self): """True when `own_reactions` returns all emoji in `ALLOWED_EMOJI`.""" - message = MagicMock(discord.Message) + message = MockMessage() own_reactions = MagicMock(return_value={"A", "B"}) with patch("bot.cogs.moderation.incidents.own_reactions", own_reactions): @@ -90,7 +88,7 @@ class TestHasSignals(unittest.TestCase): def test_has_signals_false(self): """False when `own_reactions` does not return all emoji in `ALLOWED_EMOJI`.""" - message = MagicMock(discord.Message) + message = MockMessage() own_reactions = MagicMock(return_value={"A", "C"}) with patch("bot.cogs.moderation.incidents.own_reactions", own_reactions): @@ -114,7 +112,7 @@ class TestAddSignals(unittest.IsolatedAsyncioTestCase): def setUp(self): """Prepare a mock incident message for tests to use.""" - self.incident = MagicMock(discord.Message, add_reaction=AsyncMock()) + self.incident = MockMessage() @patch("bot.cogs.moderation.incidents.own_reactions", MagicMock(return_value=set())) async def test_add_signals_missing(self): -- cgit v1.2.3 From 00a44226cb659319b9df5f568b0f67f9a0ed3360 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 15:51:34 +0200 Subject: Incidents tests: improve mock `Signal` name & move def Let's make it clear that this is our own mock. We also move the definition to the top of the module. --- tests/bot/cogs/moderation/test_incidents.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index d7cc84734..a349c1cb7 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -6,6 +6,11 @@ from bot.cogs.moderation import incidents from tests.helpers import MockMessage, MockReaction, MockTextChannel, MockUser +class MockSignal(enum.Enum): + A = "A" + B = "B" + + @patch("bot.constants.Channels.incidents", 123) class TestIsIncident(unittest.TestCase): """ @@ -95,12 +100,7 @@ class TestHasSignals(unittest.TestCase): self.assertFalse(incidents.has_signals(message)) -class Signal(enum.Enum): - A = "A" - B = "B" - - -@patch("bot.cogs.moderation.incidents.Signal", Signal) +@patch("bot.cogs.moderation.incidents.Signal", MockSignal) class TestAddSignals(unittest.IsolatedAsyncioTestCase): """ Assertions for the `add_signals` coroutine. -- cgit v1.2.3 From c66b4a618503352803f73e9272a1d27b6e0a4d52 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 17:24:31 +0200 Subject: Incidents tests: set up base class for `Incidents` For cleanliness, I've decided to make a separate class for each method. Since most tests will want to have an `Incident` instance ready, they can inherit the `setUp` from `TestIncidents`, which does not make any assertions on its own. --- tests/bot/cogs/moderation/test_incidents.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index a349c1cb7..d52932e0a 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -2,8 +2,8 @@ import enum import unittest from unittest.mock import MagicMock, call, patch -from bot.cogs.moderation import incidents -from tests.helpers import MockMessage, MockReaction, MockTextChannel, MockUser +from bot.cogs.moderation import Incidents, incidents +from tests.helpers import MockBot, MockMessage, MockReaction, MockTextChannel, MockUser class MockSignal(enum.Enum): @@ -131,3 +131,22 @@ class TestAddSignals(unittest.IsolatedAsyncioTestCase): """No emoji are added when all are present.""" await incidents.add_signals(self.incident) self.incident.add_reaction.assert_not_called() + + +class TestIncidents(unittest.IsolatedAsyncioTestCase): + """ + Tests for bound methods of the `Incidents` cog. + + Use this as a base class for `Incidents` tests - it will prepare a fresh instance + for each test function, but not make any assertions on its own. Tests can mutate + the instance as they wish. + """ + + def setUp(self): + """ + Prepare a fresh `Incidents` instance for each test. + + Note that this will not schedule `crawl_incidents` in the background, as everything + is being mocked. The `crawl_task` attribute will end up being None. + """ + self.cog_instance = Incidents(MockBot()) -- cgit v1.2.3 From 3c2d227cd067466668e3089f63a6548736edf8ab Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 17:56:31 +0200 Subject: Incidents tests: write tests for `archive` --- tests/bot/cogs/moderation/test_incidents.py | 65 ++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index d52932e0a..7500235cf 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -1,9 +1,12 @@ import enum import unittest -from unittest.mock import MagicMock, call, patch +from unittest.mock import AsyncMock, MagicMock, call, patch + +import aiohttp +import discord from bot.cogs.moderation import Incidents, incidents -from tests.helpers import MockBot, MockMessage, MockReaction, MockTextChannel, MockUser +from tests.helpers import MockAsyncWebhook, MockBot, MockMessage, MockReaction, MockTextChannel, MockUser class MockSignal(enum.Enum): @@ -150,3 +153,61 @@ class TestIncidents(unittest.IsolatedAsyncioTestCase): is being mocked. The `crawl_task` attribute will end up being None. """ self.cog_instance = Incidents(MockBot()) + + +class TestArchive(TestIncidents): + """Tests for the `Incidents.archive` coroutine.""" + + async def test_archive_webhook_not_found(self): + """ + Method recovers and returns False when the webhook is not found. + + Implicitly, this also tests that the error is handled internally and doesn't + propagate out of the method, which is just as important. + """ + mock_404 = discord.NotFound( + response=MagicMock(aiohttp.ClientResponse), # Mock the erroneous response + message="Webhook not found", + ) + + self.cog_instance.bot.fetch_webhook = AsyncMock(side_effect=mock_404) + self.assertFalse(await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock())) + + async def test_archive_relays_incident(self): + """ + If webhook is found, method relays `incident` properly. + + This test will assert the following: + * The fetched webhook's `send` method is fed the correct arguments + * The message returned by `send` will have `outcome` reaction added + * Finally, the `archive` method returns True + + Assertions are made specifically in this order. + """ + webhook_message = MockMessage() # The message that will be returned by the webhook's `send` method + webhook = MockAsyncWebhook(send=AsyncMock(return_value=webhook_message)) + + self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) # Patch in our webhook + + # Now we'll pas our own `incident` to `archive` and capture the return value + incident = MockMessage( + clean_content="pingless message", + content="pingful message", + author=MockUser(name="author_name", avatar_url="author_avatar"), + id=123, + ) + archive_return = await self.cog_instance.archive(incident, outcome=MagicMock(value="A")) + + # Check that the webhook was dispatched correctly + webhook.send.assert_called_once_with( + content="pingless message", + username="author_name", + avatar_url="author_avatar", + wait=True, + ) + + # Now check that the correct emoji was added to the relayed message + webhook_message.add_reaction.assert_called_once_with("A") + + # Finally check that the method returned True + self.assertTrue(archive_return) -- cgit v1.2.3 From 39dc3cd229888acac2782237db4b9389c0788478 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 21:52:20 +0200 Subject: Incidents tests: move `mock_404` into module namespace This will be useful for others tests as well. --- tests/bot/cogs/moderation/test_incidents.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 7500235cf..e51bda114 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -14,6 +14,12 @@ class MockSignal(enum.Enum): B = "B" +mock_404 = discord.NotFound( + response=MagicMock(aiohttp.ClientResponse), # Mock the erroneous response + message="Not found", +) + + @patch("bot.constants.Channels.incidents", 123) class TestIsIncident(unittest.TestCase): """ @@ -165,11 +171,6 @@ class TestArchive(TestIncidents): Implicitly, this also tests that the error is handled internally and doesn't propagate out of the method, which is just as important. """ - mock_404 = discord.NotFound( - response=MagicMock(aiohttp.ClientResponse), # Mock the erroneous response - message="Webhook not found", - ) - self.cog_instance.bot.fetch_webhook = AsyncMock(side_effect=mock_404) self.assertFalse(await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock())) -- cgit v1.2.3 From 8ed5cc7ef5e38885a8e439602b59e56449d3633c Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 21:52:34 +0200 Subject: Incidents tests: write tests for `resolve_message` --- tests/bot/cogs/moderation/test_incidents.py | 56 +++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index e51bda114..b3beec3ab 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -212,3 +212,59 @@ class TestArchive(TestIncidents): # Finally check that the method returned True self.assertTrue(archive_return) + + +class TestResolveMessage(TestIncidents): + """Tests for the `Incidents.resolve_message` coroutine.""" + + async def test_resolve_message_pass_message_id(self): + """Method will call `_get_message` with the passed `message_id`.""" + await self.cog_instance.resolve_message(123) + self.cog_instance.bot._connection._get_message.assert_called_once_with(123) + + async def test_resolve_message_in_cache(self): + """ + No API call is made if the queried message exists in the cache. + + We mock the `_get_message` return value regardless of input. Whether it finds the message + internally is considered d.py's responsibility, not ours. + """ + cached_message = MockMessage(id=123) + self.cog_instance.bot._connection._get_message = MagicMock(return_value=cached_message) + + return_value = await self.cog_instance.resolve_message(123) + + self.assertIs(return_value, cached_message) + self.cog_instance.bot.get_channel.assert_not_called() # The `fetch_message` line was never hit + + async def test_resolve_message_not_in_cache(self): + """ + The message is retrieved from the API if it isn't cached. + + This is desired behaviour for messages which exist, but were sent before the bot's + current session. + """ + self.cog_instance.bot._connection._get_message = MagicMock(return_value=None) # Cache returns None + + # API returns our message + uncached_message = MockMessage() + fetch_message = AsyncMock(return_value=uncached_message) + self.cog_instance.bot.get_channel = MagicMock(return_value=MockTextChannel(fetch_message=fetch_message)) + + retrieved_message = await self.cog_instance.resolve_message(123) + self.assertIs(retrieved_message, uncached_message) + + async def test_resolve_message_doesnt_exist(self): + """ + If the API returns a 404, the function handles it gracefully and returns None. + + This is an edge-case happening with racing events - event A will relay the message + to the archive and delete the original. Once event B acquires the `event_lock`, + it will not find the message in the cache, and will ask the API. + """ + self.cog_instance.bot._connection._get_message = MagicMock(return_value=None) # Cache returns None + + fetch_message = AsyncMock(side_effect=mock_404) + self.cog_instance.bot.get_channel = MagicMock(return_value=MockTextChannel(fetch_message=fetch_message)) + + self.assertIsNone(await self.cog_instance.resolve_message(123)) -- cgit v1.2.3 From bbedcb377c4c31973f43f076c3f62646f25733b3 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 22:15:38 +0200 Subject: Incidents tests: test non-404 error response --- tests/bot/cogs/moderation/test_incidents.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index b3beec3ab..cbeb3342c 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -1,4 +1,5 @@ import enum +import logging import unittest from unittest.mock import AsyncMock, MagicMock, call, patch @@ -268,3 +269,22 @@ class TestResolveMessage(TestIncidents): self.cog_instance.bot.get_channel = MagicMock(return_value=MockTextChannel(fetch_message=fetch_message)) self.assertIsNone(await self.cog_instance.resolve_message(123)) + + async def test_resolve_message_fetch_fails(self): + """ + Non-404 errors are handled, logged & None is returned. + + In contrast with a 404, this should make an error-level log. We assert that at least + one such log was made - we do not make any assertions about the log's message. + """ + self.cog_instance.bot._connection._get_message = MagicMock(return_value=None) # Cache returns None + + arbitrary_error = discord.HTTPException( + response=MagicMock(aiohttp.ClientResponse), + message="Arbitrary error", + ) + fetch_message = AsyncMock(side_effect=arbitrary_error) + self.cog_instance.bot.get_channel = MagicMock(return_value=MockTextChannel(fetch_message=fetch_message)) + + with self.assertLogs(logger=incidents.log, level=logging.ERROR): + self.assertIsNone(await self.cog_instance.resolve_message(123)) -- cgit v1.2.3 From 14b7fee42ddf6a2cc75526506ef2028bdc742c9a Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 22:42:58 +0200 Subject: Incidents tests: write tests for `on_message` --- tests/bot/cogs/moderation/test_incidents.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index cbeb3342c..0eb13df70 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -288,3 +288,30 @@ class TestResolveMessage(TestIncidents): with self.assertLogs(logger=incidents.log, level=logging.ERROR): self.assertIsNone(await self.cog_instance.resolve_message(123)) + + +class TestOnMessage(TestIncidents): + """ + Tests for the `Incidents.on_message` listener. + + Notice the decorators mocking the `is_incident` return value. The `is_incidents` + function is tested in `TestIsIncident` - here we do not worry about it. + """ + + @patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=True)) + async def test_on_message_incident(self): + """Messages qualifying as incidents are passed to `add_signals`.""" + incident = MockMessage() + + with patch("bot.cogs.moderation.incidents.add_signals", AsyncMock()) as mock_add_signals: + await self.cog_instance.on_message(incident) + + mock_add_signals.assert_called_once_with(incident) + + @patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=False)) + async def test_on_message_non_incident(self): + """Messages not qualifying as incidents are ignored.""" + with patch("bot.cogs.moderation.incidents.add_signals", AsyncMock()) as mock_add_signals: + await self.cog_instance.on_message(MockMessage()) + + mock_add_signals.assert_not_called() -- cgit v1.2.3 From 9d35846a67c2bf9ed9e935f8b5e3500ae4b49327 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 13 Jun 2020 23:24:14 +0200 Subject: Incidents tests: write tests for `make_confirmation_task` --- tests/bot/cogs/moderation/test_incidents.py | 35 +++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 0eb13df70..c093afc8a 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -215,6 +215,41 @@ class TestArchive(TestIncidents): self.assertTrue(archive_return) +class TestMakeConfirmationTask(TestIncidents): + """ + Tests for the `Incidents.make_confirmation_task` method. + + Writing tests for this method is difficult, as it mostly just delegates the provided + information elsewhere. There is very little internal logic. Whether our approach + works conceptually is difficult to prove using unit tests. + """ + + def test_make_confirmation_task_check(self): + """ + The internal check will recognize the passed incident. + + This is a little tricky - we first pass a message with a specific `id` in, and then + retrieve the built check from the `call_args` of the `wait_for` method. This relies + on the check being passed as a kwarg. + + Once the check is retrieved, we assert that it gives True for our incident's `id`, + and False for any other. + + If this function begins to fail, first check that `created_check` is being retrieved + correctly. It should be the function that is built locally in the tested method. + """ + self.cog_instance.make_confirmation_task(MockMessage(id=123)) + + self.cog_instance.bot.wait_for.assert_called_once() + created_check = self.cog_instance.bot.wait_for.call_args.kwargs["check"] + + # The `message_id` matches the `id` of our incident + self.assertTrue(created_check(payload=MagicMock(message_id=123))) + + # This `message_id` does not match + self.assertFalse(created_check(payload=MagicMock(message_id=0))) + + class TestResolveMessage(TestIncidents): """Tests for the `Incidents.resolve_message` coroutine.""" -- cgit v1.2.3 From 81e50cb2c970fc5c203e135434f897b6a3f7e52a Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 14 Jun 2020 23:34:28 -0700 Subject: Sync tests: test listeners ignore events from other guilds --- tests/bot/cogs/sync/test_cog.py | 64 ++++++++++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 10 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/sync/test_cog.py b/tests/bot/cogs/sync/test_cog.py index 14fd909c4..d7d60e961 100644 --- a/tests/bot/cogs/sync/test_cog.py +++ b/tests/bot/cogs/sync/test_cog.py @@ -131,6 +131,12 @@ class SyncCogListenerTests(SyncCogTestCase): super().setUp() self.cog.patch_user = mock.AsyncMock(spec_set=self.cog.patch_user) + self.guild_id_patcher = mock.patch("bot.cogs.sync.cog.constants.Guild.id", 5) + self.guild_id = self.guild_id_patcher.start() + + def tearDown(self): + self.guild_id_patcher.stop() + async def test_sync_cog_on_guild_role_create(self): """A POST request should be sent with the new role's data.""" self.assertTrue(self.cog.on_guild_role_create.__cog_listener__) @@ -142,20 +148,32 @@ class SyncCogListenerTests(SyncCogTestCase): "permissions": 8, "position": 23, } - role = helpers.MockRole(**role_data) + role = helpers.MockRole(**role_data, guild=self.guild_id) await self.cog.on_guild_role_create(role) self.bot.api_client.post.assert_called_once_with("bot/roles", json=role_data) + async def test_sync_cog_on_guild_role_create_ignores_guilds(self): + """Events from other guilds should be ignored.""" + role = helpers.MockRole(guild=0) + await self.cog.on_guild_role_create(role) + self.bot.api_client.post.assert_not_awaited() + async def test_sync_cog_on_guild_role_delete(self): """A DELETE request should be sent.""" self.assertTrue(self.cog.on_guild_role_delete.__cog_listener__) - role = helpers.MockRole(id=99) + role = helpers.MockRole(id=99, guild=self.guild_id) await self.cog.on_guild_role_delete(role) self.bot.api_client.delete.assert_called_once_with("bot/roles/99") + async def test_sync_cog_on_guild_role_delete_ignores_guilds(self): + """Events from other guilds should be ignored.""" + role = helpers.MockRole(guild=0) + await self.cog.on_guild_role_delete(role) + self.bot.api_client.delete.assert_not_awaited() + async def test_sync_cog_on_guild_role_update(self): """A PUT request should be sent if the colour, name, permissions, or position changes.""" self.assertTrue(self.cog.on_guild_role_update.__cog_listener__) @@ -180,8 +198,8 @@ class SyncCogListenerTests(SyncCogTestCase): after_role_data = role_data.copy() after_role_data[attribute] = 876 - before_role = helpers.MockRole(**role_data) - after_role = helpers.MockRole(**after_role_data) + before_role = helpers.MockRole(**role_data, guild=self.guild_id) + after_role = helpers.MockRole(**after_role_data, guild=self.guild_id) await self.cog.on_guild_role_update(before_role, after_role) @@ -193,11 +211,17 @@ class SyncCogListenerTests(SyncCogTestCase): else: self.bot.api_client.put.assert_not_called() + async def test_sync_cog_on_guild_role_update_ignores_guilds(self): + """Events from other guilds should be ignored.""" + role = helpers.MockRole(guild=0) + await self.cog.on_guild_role_update(role, role) + self.bot.api_client.put.assert_not_awaited() + async def test_sync_cog_on_member_remove(self): - """Member should patched to set in_guild as False.""" + """Member should be patched to set in_guild as False.""" self.assertTrue(self.cog.on_member_remove.__cog_listener__) - member = helpers.MockMember() + member = helpers.MockMember(guild=self.guild_id) await self.cog.on_member_remove(member) self.cog.patch_user.assert_called_once_with( @@ -205,14 +229,20 @@ class SyncCogListenerTests(SyncCogTestCase): updated_information={"in_guild": False} ) + async def test_sync_cog_on_member_remove_ignores_guilds(self): + """Events from other guilds should be ignored.""" + member = helpers.MockMember(guild=0) + await self.cog.on_member_remove(member) + self.cog.patch_user.assert_not_awaited() + async def test_sync_cog_on_member_update_roles(self): """Members should be patched if their roles have changed.""" self.assertTrue(self.cog.on_member_update.__cog_listener__) # Roles are intentionally unsorted. before_roles = [helpers.MockRole(id=12), helpers.MockRole(id=30), helpers.MockRole(id=20)] - before_member = helpers.MockMember(roles=before_roles) - after_member = helpers.MockMember(roles=before_roles[1:]) + before_member = helpers.MockMember(roles=before_roles, guild=self.guild_id) + after_member = helpers.MockMember(roles=before_roles[1:], guild=self.guild_id) await self.cog.on_member_update(before_member, after_member) @@ -233,13 +263,19 @@ class SyncCogListenerTests(SyncCogTestCase): with self.subTest(attribute=attribute): self.cog.patch_user.reset_mock() - before_member = helpers.MockMember(**{attribute: old_value}) - after_member = helpers.MockMember(**{attribute: new_value}) + before_member = helpers.MockMember(**{attribute: old_value}, guild=self.guild_id) + after_member = helpers.MockMember(**{attribute: new_value}, guild=self.guild_id) await self.cog.on_member_update(before_member, after_member) self.cog.patch_user.assert_not_called() + async def test_sync_cog_on_member_update_ignores_guilds(self): + """Events from other guilds should be ignored.""" + member = helpers.MockMember(guild=0) + await self.cog.on_member_update(member, member) + self.cog.patch_user.assert_not_awaited() + async def test_sync_cog_on_user_update(self): """A user should be patched only if the name, discriminator, or avatar changes.""" self.assertTrue(self.cog.on_user_update.__cog_listener__) @@ -290,6 +326,7 @@ class SyncCogListenerTests(SyncCogTestCase): member = helpers.MockMember( discriminator="1234", roles=[helpers.MockRole(id=22), helpers.MockRole(id=12)], + guild=self.guild_id, ) data = { @@ -334,6 +371,13 @@ class SyncCogListenerTests(SyncCogTestCase): self.bot.api_client.post.assert_not_called() + async def test_sync_cog_on_member_join_ignores_guilds(self): + """Events from other guilds should be ignored.""" + member = helpers.MockMember(guild=0) + await self.cog.on_member_join(member) + self.bot.api_client.post.assert_not_awaited() + self.bot.api_client.put.assert_not_awaited() + class SyncCogCommandTests(SyncCogTestCase, CommandTestCase): """Tests for the commands in the Sync cog.""" -- cgit v1.2.3 From 4d6acdf32a323de8b88fed464358d70faf35c9d1 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 14 Jun 2020 23:47:40 -0700 Subject: Sync: ignore 404s in on_user_update 404s probably mean the user is from another guild. --- bot/cogs/sync/cog.py | 14 ++++++++------ tests/bot/cogs/sync/test_cog.py | 17 ++++++++++------- 2 files changed, 18 insertions(+), 13 deletions(-) (limited to 'tests') diff --git a/bot/cogs/sync/cog.py b/bot/cogs/sync/cog.py index 97ea31ba5..578cccfc9 100644 --- a/bot/cogs/sync/cog.py +++ b/bot/cogs/sync/cog.py @@ -34,14 +34,15 @@ class Sync(Cog): for syncer in (self.role_syncer, self.user_syncer): await syncer.sync(guild) - async def patch_user(self, user_id: int, updated_information: Dict[str, Any]) -> None: + async def patch_user(self, user_id: int, json: Dict[str, Any], ignore_404: bool = False) -> None: """Send a PATCH request to partially update a user in the database.""" try: - await self.bot.api_client.patch(f"bot/users/{user_id}", json=updated_information) + await self.bot.api_client.patch(f"bot/users/{user_id}", json=json) except ResponseCodeError as e: if e.response.status != 404: raise - log.warning("Unable to update user, got 404. Assuming race condition from join event.") + if not ignore_404: + log.warning("Unable to update user, got 404. Assuming race condition from join event.") @Cog.listener() async def on_guild_role_create(self, role: Role) -> None: @@ -137,7 +138,7 @@ class Sync(Cog): if member.guild != constants.Guild.id: return - await self.patch_user(member.id, updated_information={"in_guild": False}) + await self.patch_user(member.id, json={"in_guild": False}) @Cog.listener() async def on_member_update(self, before: Member, after: Member) -> None: @@ -147,7 +148,7 @@ class Sync(Cog): if before.roles != after.roles: updated_information = {"roles": sorted(role.id for role in after.roles)} - await self.patch_user(after.id, updated_information=updated_information) + await self.patch_user(after.id, json=updated_information) @Cog.listener() async def on_user_update(self, before: User, after: User) -> None: @@ -158,7 +159,8 @@ class Sync(Cog): "name": after.name, "discriminator": int(after.discriminator), } - await self.patch_user(after.id, updated_information=updated_information) + # A 404 likely means the user is in another guild. + await self.patch_user(after.id, json=updated_information, ignore_404=True) @commands.group(name='sync') @commands.has_permissions(administrator=True) diff --git a/tests/bot/cogs/sync/test_cog.py b/tests/bot/cogs/sync/test_cog.py index d7d60e961..e5be14391 100644 --- a/tests/bot/cogs/sync/test_cog.py +++ b/tests/bot/cogs/sync/test_cog.py @@ -226,7 +226,7 @@ class SyncCogListenerTests(SyncCogTestCase): self.cog.patch_user.assert_called_once_with( member.id, - updated_information={"in_guild": False} + json={"in_guild": False} ) async def test_sync_cog_on_member_remove_ignores_guilds(self): @@ -247,7 +247,7 @@ class SyncCogListenerTests(SyncCogTestCase): await self.cog.on_member_update(before_member, after_member) data = {"roles": sorted(role.id for role in after_member.roles)} - self.cog.patch_user.assert_called_once_with(after_member.id, updated_information=data) + self.cog.patch_user.assert_called_once_with(after_member.id, json=data) async def test_sync_cog_on_member_update_other(self): """Members should not be patched if other attributes have changed.""" @@ -308,12 +308,15 @@ class SyncCogListenerTests(SyncCogTestCase): # Don't care if *all* keys are present; only the changed one is required call_args = self.cog.patch_user.call_args - self.assertEqual(call_args[0][0], after_user.id) - self.assertIn("updated_information", call_args[1]) + self.assertEqual(call_args.args[0], after_user.id) + self.assertIn("json", call_args.kwargs) - updated_information = call_args[1]["updated_information"] - self.assertIn(api_field, updated_information) - self.assertEqual(updated_information[api_field], api_value) + self.assertIn("ignore_404", call_args.kwargs) + self.assertTrue(call_args.kwargs["ignore_404"]) + + json = call_args.kwargs["json"] + self.assertIn(api_field, json) + self.assertEqual(json[api_field], api_value) else: self.cog.patch_user.assert_not_called() -- cgit v1.2.3 From ebc0eae42c1da67f61f040a67bc1b70e53a6f97e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 17 Jun 2020 16:46:43 -0700 Subject: Sync: fix guild ID check Need to compare the IDs against each other rather than the Guild object against the ID. --- bot/cogs/sync/cog.py | 12 ++++++------ tests/bot/cogs/sync/test_cog.py | 35 +++++++++++++++++++---------------- 2 files changed, 25 insertions(+), 22 deletions(-) (limited to 'tests') diff --git a/bot/cogs/sync/cog.py b/bot/cogs/sync/cog.py index 578cccfc9..5ace957e7 100644 --- a/bot/cogs/sync/cog.py +++ b/bot/cogs/sync/cog.py @@ -47,7 +47,7 @@ class Sync(Cog): @Cog.listener() async def on_guild_role_create(self, role: Role) -> None: """Adds newly create role to the database table over the API.""" - if role.guild != constants.Guild.id: + if role.guild.id != constants.Guild.id: return await self.bot.api_client.post( @@ -64,7 +64,7 @@ class Sync(Cog): @Cog.listener() async def on_guild_role_delete(self, role: Role) -> None: """Deletes role from the database when it's deleted from the guild.""" - if role.guild != constants.Guild.id: + if role.guild.id != constants.Guild.id: return await self.bot.api_client.delete(f'bot/roles/{role.id}') @@ -72,7 +72,7 @@ class Sync(Cog): @Cog.listener() async def on_guild_role_update(self, before: Role, after: Role) -> None: """Syncs role with the database if any of the stored attributes were updated.""" - if after.guild != constants.Guild.id: + if after.guild.id != constants.Guild.id: return was_updated = ( @@ -103,7 +103,7 @@ class Sync(Cog): previously left), it will update the user's information. If the user is not yet known by the database, the user is added. """ - if member.guild != constants.Guild.id: + if member.guild.id != constants.Guild.id: return packed = { @@ -135,7 +135,7 @@ class Sync(Cog): @Cog.listener() async def on_member_remove(self, member: Member) -> None: """Set the in_guild field to False when a member leaves the guild.""" - if member.guild != constants.Guild.id: + if member.guild.id != constants.Guild.id: return await self.patch_user(member.id, json={"in_guild": False}) @@ -143,7 +143,7 @@ class Sync(Cog): @Cog.listener() async def on_member_update(self, before: Member, after: Member) -> None: """Update the roles of the member in the database if a change is detected.""" - if after.guild != constants.Guild.id: + if after.guild.id != constants.Guild.id: return if before.roles != after.roles: diff --git a/tests/bot/cogs/sync/test_cog.py b/tests/bot/cogs/sync/test_cog.py index e5be14391..120bc991d 100644 --- a/tests/bot/cogs/sync/test_cog.py +++ b/tests/bot/cogs/sync/test_cog.py @@ -134,6 +134,9 @@ class SyncCogListenerTests(SyncCogTestCase): self.guild_id_patcher = mock.patch("bot.cogs.sync.cog.constants.Guild.id", 5) self.guild_id = self.guild_id_patcher.start() + self.guild = helpers.MockGuild(id=self.guild_id) + self.other_guild = helpers.MockGuild(id=0) + def tearDown(self): self.guild_id_patcher.stop() @@ -148,14 +151,14 @@ class SyncCogListenerTests(SyncCogTestCase): "permissions": 8, "position": 23, } - role = helpers.MockRole(**role_data, guild=self.guild_id) + role = helpers.MockRole(**role_data, guild=self.guild) await self.cog.on_guild_role_create(role) self.bot.api_client.post.assert_called_once_with("bot/roles", json=role_data) async def test_sync_cog_on_guild_role_create_ignores_guilds(self): """Events from other guilds should be ignored.""" - role = helpers.MockRole(guild=0) + role = helpers.MockRole(guild=self.other_guild) await self.cog.on_guild_role_create(role) self.bot.api_client.post.assert_not_awaited() @@ -163,14 +166,14 @@ class SyncCogListenerTests(SyncCogTestCase): """A DELETE request should be sent.""" self.assertTrue(self.cog.on_guild_role_delete.__cog_listener__) - role = helpers.MockRole(id=99, guild=self.guild_id) + role = helpers.MockRole(id=99, guild=self.guild) await self.cog.on_guild_role_delete(role) self.bot.api_client.delete.assert_called_once_with("bot/roles/99") async def test_sync_cog_on_guild_role_delete_ignores_guilds(self): """Events from other guilds should be ignored.""" - role = helpers.MockRole(guild=0) + role = helpers.MockRole(guild=self.other_guild) await self.cog.on_guild_role_delete(role) self.bot.api_client.delete.assert_not_awaited() @@ -198,8 +201,8 @@ class SyncCogListenerTests(SyncCogTestCase): after_role_data = role_data.copy() after_role_data[attribute] = 876 - before_role = helpers.MockRole(**role_data, guild=self.guild_id) - after_role = helpers.MockRole(**after_role_data, guild=self.guild_id) + before_role = helpers.MockRole(**role_data, guild=self.guild) + after_role = helpers.MockRole(**after_role_data, guild=self.guild) await self.cog.on_guild_role_update(before_role, after_role) @@ -213,7 +216,7 @@ class SyncCogListenerTests(SyncCogTestCase): async def test_sync_cog_on_guild_role_update_ignores_guilds(self): """Events from other guilds should be ignored.""" - role = helpers.MockRole(guild=0) + role = helpers.MockRole(guild=self.other_guild) await self.cog.on_guild_role_update(role, role) self.bot.api_client.put.assert_not_awaited() @@ -221,7 +224,7 @@ class SyncCogListenerTests(SyncCogTestCase): """Member should be patched to set in_guild as False.""" self.assertTrue(self.cog.on_member_remove.__cog_listener__) - member = helpers.MockMember(guild=self.guild_id) + member = helpers.MockMember(guild=self.guild) await self.cog.on_member_remove(member) self.cog.patch_user.assert_called_once_with( @@ -231,7 +234,7 @@ class SyncCogListenerTests(SyncCogTestCase): async def test_sync_cog_on_member_remove_ignores_guilds(self): """Events from other guilds should be ignored.""" - member = helpers.MockMember(guild=0) + member = helpers.MockMember(guild=self.other_guild) await self.cog.on_member_remove(member) self.cog.patch_user.assert_not_awaited() @@ -241,8 +244,8 @@ class SyncCogListenerTests(SyncCogTestCase): # Roles are intentionally unsorted. before_roles = [helpers.MockRole(id=12), helpers.MockRole(id=30), helpers.MockRole(id=20)] - before_member = helpers.MockMember(roles=before_roles, guild=self.guild_id) - after_member = helpers.MockMember(roles=before_roles[1:], guild=self.guild_id) + before_member = helpers.MockMember(roles=before_roles, guild=self.guild) + after_member = helpers.MockMember(roles=before_roles[1:], guild=self.guild) await self.cog.on_member_update(before_member, after_member) @@ -263,8 +266,8 @@ class SyncCogListenerTests(SyncCogTestCase): with self.subTest(attribute=attribute): self.cog.patch_user.reset_mock() - before_member = helpers.MockMember(**{attribute: old_value}, guild=self.guild_id) - after_member = helpers.MockMember(**{attribute: new_value}, guild=self.guild_id) + before_member = helpers.MockMember(**{attribute: old_value}, guild=self.guild) + after_member = helpers.MockMember(**{attribute: new_value}, guild=self.guild) await self.cog.on_member_update(before_member, after_member) @@ -272,7 +275,7 @@ class SyncCogListenerTests(SyncCogTestCase): async def test_sync_cog_on_member_update_ignores_guilds(self): """Events from other guilds should be ignored.""" - member = helpers.MockMember(guild=0) + member = helpers.MockMember(guild=self.other_guild) await self.cog.on_member_update(member, member) self.cog.patch_user.assert_not_awaited() @@ -329,7 +332,7 @@ class SyncCogListenerTests(SyncCogTestCase): member = helpers.MockMember( discriminator="1234", roles=[helpers.MockRole(id=22), helpers.MockRole(id=12)], - guild=self.guild_id, + guild=self.guild, ) data = { @@ -376,7 +379,7 @@ class SyncCogListenerTests(SyncCogTestCase): async def test_sync_cog_on_member_join_ignores_guilds(self): """Events from other guilds should be ignored.""" - member = helpers.MockMember(guild=0) + member = helpers.MockMember(guild=self.other_guild) await self.cog.on_member_join(member) self.bot.api_client.post.assert_not_awaited() self.bot.api_client.put.assert_not_awaited() -- cgit v1.2.3 From 40e00ff17465fc5a5fe6b46487bfea37655cd7b9 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Thu, 18 Jun 2020 19:33:59 +0200 Subject: Incidents tests: write tests for `process_event` This also breaks the helpers import statement into a vertical list, as the amount of imports has grown too much. I still believe that this is a preferred alternative to accessing the helpers via module namespace, as we use them a lot, and the added visual noise would be annoying to read - their names are already descriptive enough. --- tests/bot/cogs/moderation/test_incidents.py | 102 +++++++++++++++++++++++++++- 1 file changed, 101 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index c093afc8a..6158d5d20 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -1,3 +1,4 @@ +import asyncio import enum import logging import unittest @@ -7,7 +8,16 @@ import aiohttp import discord from bot.cogs.moderation import Incidents, incidents -from tests.helpers import MockAsyncWebhook, MockBot, MockMessage, MockReaction, MockTextChannel, MockUser +from tests.helpers import ( + MockAsyncWebhook, + MockBot, + MockMember, + MockMessage, + MockReaction, + MockRole, + MockTextChannel, + MockUser, +) class MockSignal(enum.Enum): @@ -250,6 +260,96 @@ class TestMakeConfirmationTask(TestIncidents): self.assertFalse(created_check(payload=MagicMock(message_id=0))) +@patch("bot.cogs.moderation.incidents.ALLOWED_ROLES", {1, 2}) +@patch("bot.cogs.moderation.incidents.Incidents.make_confirmation_task", AsyncMock()) # Generic awaitable +class TestProcessEvent(TestIncidents): + """Tests for the `Incidents.process_event` coroutine.""" + + @patch("bot.cogs.moderation.incidents.ALLOWED_ROLES", {1, 2}) + async def test_process_event_bad_role(self): + """The reaction is removed when the author lacks all allowed roles.""" + incident = MockMessage() + member = MockMember(roles=[MockRole(id=0)]) # Must have role 1 or 2 + + await self.cog_instance.process_event("reaction", incident, member) + incident.remove_reaction.assert_called_once_with("reaction", member) + + async def test_process_event_bad_emoji(self): + """ + The reaction is removed when an invalid emoji is used. + + This requires that we pass in a `member` with valid roles, as we need the role check + to succeed. + """ + incident = MockMessage() + member = MockMember(roles=[MockRole(id=1)]) # Member has allowed role + + await self.cog_instance.process_event("invalid_signal", incident, member) + incident.remove_reaction.assert_called_once_with("invalid_signal", member) + + async def test_process_event_no_archive_on_investigating(self): + """Message is not archived on `Signal.INVESTIGATING`.""" + with patch("bot.cogs.moderation.incidents.Incidents.archive", AsyncMock()) as mocked_archive: + await self.cog_instance.process_event( + reaction=incidents.Signal.INVESTIGATING.value, + incident=MockMessage(), + member=MockMember(roles=[MockRole(id=1)]), + ) + + mocked_archive.assert_not_called() + + async def test_process_event_no_delete_if_archive_fails(self): + """ + Original message is not deleted when `Incidents.archive` returns False. + + This is the way of signaling that the relay failed, and we should not remove the original, + as that would result in losing the incident record. + """ + incident = MockMessage() + + with patch("bot.cogs.moderation.incidents.Incidents.archive", AsyncMock(return_value=False)): + await self.cog_instance.process_event( + reaction=incidents.Signal.ACTIONED.value, + incident=incident, + member=MockMember(roles=[MockRole(id=1)]) + ) + + incident.delete.assert_not_called() + + async def test_process_event_confirmation_task_is_awaited(self): + """Task given by `Incidents.make_confirmation_task` is awaited before method exits.""" + mock_task = AsyncMock() + + with patch("bot.cogs.moderation.incidents.Incidents.make_confirmation_task", mock_task): + await self.cog_instance.process_event( + reaction=incidents.Signal.ACTIONED.value, + incident=MockMessage(), + member=MockMember(roles=[MockRole(id=1)]) + ) + + mock_task.assert_awaited() + + async def test_process_event_confirmation_task_timeout_is_handled(self): + """ + Confirmation task `asyncio.TimeoutError` is handled gracefully. + + We have `make_confirmation_task` return a mock with a side effect, and then catch the + exception should it propagate out of `process_event`. This is so that we can then manually + fail the test with a more informative message than just the plain traceback. + """ + mock_task = AsyncMock(side_effect=asyncio.TimeoutError()) + + try: + with patch("bot.cogs.moderation.incidents.Incidents.make_confirmation_task", mock_task): + await self.cog_instance.process_event( + reaction=incidents.Signal.ACTIONED.value, + incident=MockMessage(), + member=MockMember(roles=[MockRole(id=1)]) + ) + except asyncio.TimeoutError: + self.fail("TimeoutError was not handled gracefully, and propagated out of `process_event`!") + + class TestResolveMessage(TestIncidents): """Tests for the `Incidents.resolve_message` coroutine.""" -- cgit v1.2.3 From ed4097629601704f0c65fc40cceb5fd6757d4779 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 19 Jun 2020 14:32:31 +0200 Subject: Incidents tests: add helper for mocking async for-loops See the docstring. This does not make the ambition to be powerful enough to be included in `tests.helpers`, and is only intended for local purposes. --- tests/bot/cogs/moderation/test_incidents.py | 37 +++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 6158d5d20..7fa8847ef 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -1,6 +1,7 @@ import asyncio import enum import logging +import typing as t import unittest from unittest.mock import AsyncMock, MagicMock, call, patch @@ -20,6 +21,42 @@ from tests.helpers import ( ) +class MockAsyncIterable: + """ + Helper for mocking asynchronous for loops. + + It does not appear that the `unittest` library currently provides anything that would + allow us to simply mock an async iterator, such as `discord.TextChannel.history`. + + We therefore write our own helper to wrap a regular synchronous iterable, and feed + its values via `__anext__` rather than `__next__`. + + This class was written for the purposes of testing the `Incidents` cog - it may not + be generic enough to be placed in the `tests.helpers` module. + """ + + def __init__(self, messages: t.Iterable): + """Take a sync iterable to be wrapped.""" + self.iter_messages = iter(messages) + + def __aiter__(self): + """Return `self` as we provide the `__anext__` method.""" + return self + + async def __anext__(self): + """ + Feed the next item, or raise `StopAsyncIteration`. + + Since we're wrapping a sync iterator, it will communicate that it has been depleted + by raising a `StopIteration`. The `async for` construct does not expect it, and we + therefore need to substitute it for the appropriate exception type. + """ + try: + return next(self.iter_messages) + except StopIteration: + raise StopAsyncIteration + + class MockSignal(enum.Enum): A = "A" B = "B" -- cgit v1.2.3 From d93ed5d801c08b7fb084427906e7ac484ac3563f Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 19 Jun 2020 14:37:44 +0200 Subject: Incidents tests: write tests for `crawl_incidents` --- tests/bot/cogs/moderation/test_incidents.py | 58 +++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 7fa8847ef..4e6dfd5f7 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -209,6 +209,64 @@ class TestIncidents(unittest.IsolatedAsyncioTestCase): self.cog_instance = Incidents(MockBot()) +@patch("asyncio.sleep", AsyncMock()) # Prevent the coro from sleeping to speed up the test +class TestCrawlIncidents(TestIncidents): + """ + Tests for the `Incidents.crawl_incidents` coroutine. + + Apart from `test_crawl_incidents_waits_until_cache_ready`, all tests in this class + will patch the return values of `is_incident` and `has_signal` and then observe + whether the `AsyncMock` for `add_signals` was awaited or not. + + The `add_signals` mock is added by each test separately to ensure it is clean (has not + been awaited by another test yet). The mock can be reset, but this appears to be the + cleaner way. + + For each test, we inject a mock channel with a history of 1 message only (see: `setUp`). + """ + + def setUp(self): + """For each test, ensure `bot.get_channel` returns a channel with 1 arbitrary message.""" + super().setUp() # First ensure we get `cog_instance` from parent + + incidents_history = MagicMock(return_value=MockAsyncIterable([MockMessage()])) + self.cog_instance.bot.get_channel = MagicMock(return_value=MockTextChannel(history=incidents_history)) + + async def test_crawl_incidents_waits_until_cache_ready(self): + """ + The coroutine will await the `wait_until_guild_available` event. + + Since this task is schedule in the `__init__`, it is critical that it waits for the + cache to be ready, so that it can safely get the #incidents channel. + """ + await self.cog_instance.crawl_incidents() + self.cog_instance.bot.wait_until_guild_available.assert_awaited() + + @patch("bot.cogs.moderation.incidents.add_signals", AsyncMock()) + @patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=False)) # Message doesn't qualify + @patch("bot.cogs.moderation.incidents.has_signals", MagicMock(return_value=False)) + async def test_crawl_incidents_noop_if_is_not_incident(self): + """Signals are not added for a non-incident message.""" + await self.cog_instance.crawl_incidents() + incidents.add_signals.assert_not_awaited() + + @patch("bot.cogs.moderation.incidents.add_signals", AsyncMock()) + @patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=True)) # Message qualifies + @patch("bot.cogs.moderation.incidents.has_signals", MagicMock(return_value=True)) # But already has signals + async def test_crawl_incidents_noop_if_message_already_has_signals(self): + """Signals are not added for messages which already have them.""" + await self.cog_instance.crawl_incidents() + incidents.add_signals.assert_not_awaited() + + @patch("bot.cogs.moderation.incidents.add_signals", AsyncMock()) + @patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=True)) # Message qualifies + @patch("bot.cogs.moderation.incidents.has_signals", MagicMock(return_value=False)) # And doesn't have signals + async def test_crawl_incidents_add_signals_called(self): + """Message has signals added as it does not have them yet and qualifies as an incident.""" + await self.cog_instance.crawl_incidents() + incidents.add_signals.assert_awaited_once() + + class TestArchive(TestIncidents): """Tests for the `Incidents.archive` coroutine.""" -- cgit v1.2.3 From 9a58b45cad51c961ad34fa9de9aaa060446c54fd Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 19 Jun 2020 16:57:15 +0200 Subject: Incidents tests: write tests for `on_raw_reaction_add` --- tests/bot/cogs/moderation/test_incidents.py | 128 ++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 4e6dfd5f7..55b15ec9e 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -520,6 +520,134 @@ class TestResolveMessage(TestIncidents): self.assertIsNone(await self.cog_instance.resolve_message(123)) +@patch("bot.constants.Channels.incidents", 123) +class TestOnRawReactionAdd(TestIncidents): + """ + Tests for the `Incidents.on_raw_reaction_add` listener. + + Writing tests for this listener comes with additional complexity due to the listener + awaiting the `crawl_task` task. See `asyncSetUp` for further details, which attempts + to make unit testing this function possible. + """ + + def setUp(self): + """ + Prepare & assign `payload` attribute. + + This attribute represents an *ideal* payload which will not be rejected by the + listener. As each test will receive a fresh instance, it can be mutated to + observe how the listener's behaviour changes with different attributes on + the passed payload. + """ + super().setUp() # Ensure `cog_instance` is assigned + + self.payload = MagicMock( + discord.RawReactionActionEvent, + channel_id=123, # Patched at class level + message_id=456, + member=MockMember(bot=False), + emoji="reaction", + ) + + async def asyncSetUp(self): # noqa: N802 + """ + Prepare an empty task and assign it as `crawl_task`. + + It appears that the `unittest` framework does not provide anything for mocking + asyncio tasks. An `AsyncMock` instance can be called and then awaited, however, + it does not provide the `done` method or any other parts of the `asyncio.Task` + interface. + + Although we do not need to make any assertions about the task itself while + testing the listener, the code will still await it and call the `done` method, + and so we must inject something that will not fail on either action. + + Note that this is done in an `asyncSetUp`, which runs after `setUp`. + The justification is that creating an actual task requires the event + loop to be ready, which is not the case in the `setUp`. + """ + mock_task = asyncio.create_task(AsyncMock()()) # Mock async func, then a coro + self.cog_instance.crawl_task = mock_task + + async def test_on_raw_reaction_add_wrong_channel(self): + """ + Events outside of #incidents will be ignored. + + We check this by asserting that `resolve_message` was never queried. + """ + self.payload.channel_id = 0 + self.cog_instance.resolve_message = AsyncMock() + + await self.cog_instance.on_raw_reaction_add(self.payload) + self.cog_instance.resolve_message.assert_not_called() + + async def test_on_raw_reaction_add_user_is_bot(self): + """ + Events dispatched by bot accounts will be ignored. + + We check this by asserting that `resolve_message` was never queried. + """ + self.payload.member = MockMember(bot=True) + self.cog_instance.resolve_message = AsyncMock() + + await self.cog_instance.on_raw_reaction_add(self.payload) + self.cog_instance.resolve_message.assert_not_called() + + async def test_on_raw_reaction_add_message_doesnt_exist(self): + """ + Listener gracefully handles the case where `resolve_message` gives None. + + We check this by asserting that `process_event` was never called. + """ + self.cog_instance.process_event = AsyncMock() + self.cog_instance.resolve_message = AsyncMock(return_value=None) + + await self.cog_instance.on_raw_reaction_add(self.payload) + self.cog_instance.process_event.assert_not_called() + + async def test_on_raw_reaction_add_message_is_not_an_incident(self): + """ + The event won't be processed if the related message is not an incident. + + This is an edge-case that can happen if someone manually leaves a reaction + on a pinned message, or a comment. + + We check this by asserting that `process_event` was never called. + """ + self.cog_instance.process_event = AsyncMock() + self.cog_instance.resolve_message = AsyncMock(return_value=MockMessage()) + + with patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=False)): + await self.cog_instance.on_raw_reaction_add(self.payload) + + self.cog_instance.process_event.assert_not_called() + + async def test_on_raw_reaction_add_valid_event_is_processed(self): + """ + If the reaction event is valid, it is passed to `process_event`. + + This is the case when everything goes right: + * The reaction was placed in #incidents, and not by a bot + * The message was found successfully + * The message qualifies as an incident + + Additionally, we check that all arguments were passed as expected. + """ + incident = MockMessage(id=1) + + self.cog_instance.process_event = AsyncMock() + self.cog_instance.resolve_message = AsyncMock(return_value=incident) + + with patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=True)): + await self.cog_instance.on_raw_reaction_add(self.payload) + + self.cog_instance.process_event.assert_called_with( + "reaction", # Defined in `self.payload` + incident, + self.payload.member, + ) + + class TestOnMessage(TestIncidents): """ Tests for the `Incidents.on_message` listener. -- cgit v1.2.3 From 98b8947ab7865e33f18da8e2a62b26405676e8e4 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 20 Jun 2020 13:13:45 +0200 Subject: Incidents: try-except Signal creation Suggested by Mark during review. This follows the "ask for forgiveness rather than permission" paradigm, ends up being less code to read, and may be seen as more logical / safer. The `ALLOWED_EMOJI` set was renamed to `ALL_SIGNALS` as this now better communicates the set's purpose. Tests adjusted as appropriate. Co-authored-by: MarkKoz --- bot/cogs/moderation/incidents.py | 18 +++++++++++------- tests/bot/cogs/moderation/test_incidents.py | 8 ++++---- 2 files changed, 15 insertions(+), 11 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 089a5bc9f..41a98bcb7 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -33,9 +33,11 @@ class Signal(Enum): INVESTIGATING = Emojis.incident_investigating -# Reactions from roles not listed here, or using emoji not listed here, will be removed +# Reactions from roles not listed here will be removed ALLOWED_ROLES: t.Set[int] = {Roles.moderators, Roles.admins, Roles.owners} -ALLOWED_EMOJI: t.Set[str] = {signal.value for signal in Signal} + +# Message must have all of these emoji to pass the `has_signals` check +ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal} def is_incident(message: discord.Message) -> bool: @@ -56,7 +58,7 @@ def own_reactions(message: discord.Message) -> t.Set[str]: def has_signals(message: discord.Message) -> bool: """True if `message` already has all `Signal` reactions, False otherwise.""" - return ALLOWED_EMOJI.issubset(own_reactions(message)) + return ALL_SIGNALS.issubset(own_reactions(message)) async def add_signals(incident: discord.Message) -> None: @@ -96,7 +98,9 @@ class Incidents(Cog): * See: `on_message` On reaction: - * Remove reaction if not permitted (`ALLOWED_EMOJI`, `ALLOWED_ROLES`) + * Remove reaction if not permitted + * User does not have any of the roles in `ALLOWED_ROLES` + * Used emoji is not a `Signal` member * If `Signal.ACTIONED` or `Signal.NOT_ACTIONED` were chosen, attempt to relay the incident message to #incidents-archive * If relay successful, delete original message @@ -217,13 +221,13 @@ class Incidents(Cog): await incident.remove_reaction(reaction, member) return - if reaction not in ALLOWED_EMOJI: + try: + signal = Signal(reaction) + except ValueError: log.debug(f"Removing invalid reaction: emoji {reaction} is not a valid signal") await incident.remove_reaction(reaction, member) return - # If we reach this point, we know that `emoji` is a `Signal` member - signal = Signal(reaction) log.trace(f"Received signal: {signal}") if signal not in (Signal.ACTIONED, Signal.NOT_ACTIONED): diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 55b15ec9e..862736785 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -131,17 +131,17 @@ class TestOwnReactions(unittest.TestCase): self.assertSetEqual(incidents.own_reactions(message), {"A", "B"}) -@patch("bot.cogs.moderation.incidents.ALLOWED_EMOJI", {"A", "B"}) +@patch("bot.cogs.moderation.incidents.ALL_SIGNALS", {"A", "B"}) class TestHasSignals(unittest.TestCase): """ Assertions for the `has_signals` function. - We patch `ALLOWED_EMOJI` globally. Each test function then patches `own_reactions` + We patch `ALL_SIGNALS` globally. Each test function then patches `own_reactions` as appropriate. """ def test_has_signals_true(self): - """True when `own_reactions` returns all emoji in `ALLOWED_EMOJI`.""" + """True when `own_reactions` returns all emoji in `ALL_SIGNALS`.""" message = MockMessage() own_reactions = MagicMock(return_value={"A", "B"}) @@ -149,7 +149,7 @@ class TestHasSignals(unittest.TestCase): self.assertTrue(incidents.has_signals(message)) def test_has_signals_false(self): - """False when `own_reactions` does not return all emoji in `ALLOWED_EMOJI`.""" + """False when `own_reactions` does not return all emoji in `ALL_SIGNALS`.""" message = MockMessage() own_reactions = MagicMock(return_value={"A", "C"}) -- cgit v1.2.3 From 20dbd177f227511b9c3cb678ab45a67558cd3d7f Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 20 Jun 2020 13:15:43 +0200 Subject: Incidents tests: remove unnecessary patch This is already being patched at class-level. --- tests/bot/cogs/moderation/test_incidents.py | 1 - 1 file changed, 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 862736785..9f0553216 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -360,7 +360,6 @@ class TestMakeConfirmationTask(TestIncidents): class TestProcessEvent(TestIncidents): """Tests for the `Incidents.process_event` coroutine.""" - @patch("bot.cogs.moderation.incidents.ALLOWED_ROLES", {1, 2}) async def test_process_event_bad_role(self): """The reaction is removed when the author lacks all allowed roles.""" incident = MockMessage() -- cgit v1.2.3 From f240a970c6b97d201959d25a79a8babafed1c2b1 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 20 Jun 2020 14:15:34 +0200 Subject: Incidents tests: assert webhook username is de-clyded See: a8b4e394d9da57287cd9497cd9bb0a97fa467e84 --- tests/bot/cogs/moderation/test_incidents.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 9f0553216..2fc9180cf 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -319,6 +319,25 @@ class TestArchive(TestIncidents): # Finally check that the method returned True self.assertTrue(archive_return) + async def test_archive_clyde_username(self): + """ + The archive webhook username is cleansed using `sub_clyde`. + + Discord will reject any webhook with "clyde" in the username field, as it impersonates + the official Clyde bot. Since we do not control what the username will be (the incident + author name is used), we must ensure the name is cleansed, otherwise the relay may fail. + + This test assumes the username is passed as a kwarg. If this test fails, please review + whether the passed argument is being retrieved correctly. + """ + webhook = MockAsyncWebhook() + self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) + + message_from_clyde = MockMessage(author=MockUser(name="clyde the great")) + await self.cog_instance.archive(message_from_clyde, MagicMock(incidents.Signal)) + + self.assertNotIn("clyde", webhook.send.call_args.kwargs["username"]) + class TestMakeConfirmationTask(TestIncidents): """ -- cgit v1.2.3 From 6fa8caed037b247a7c194f58a4635de7dae21fd2 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sun, 21 Jun 2020 13:51:17 +0200 Subject: Incidents: implement `make_username` helper The justification is to incorporate the `actioned_by` name into the username in some way, and so the logical thing to do is to abstract this process into a helper so that it can easily be adjusted in the future. For now, I've chosen to separate the names by a pipe. Discord webhook username cannot exceed 80 characters in length, and so we cap it at this length by default. This is seen as more of an edge-case, but it should be accounted for, as we're not joining two names. The `max_length` param is configurable primarily for testing purposes, it probably should never be passed explicitly. This commit also provides two tests for the function. --- bot/cogs/moderation/incidents.py | 24 ++++++++++++++++++++++++ tests/bot/cogs/moderation/test_incidents.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 040f2c0c8..2cce9b6fe 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -41,6 +41,30 @@ ALLOWED_ROLES: t.Set[int] = {Roles.moderators, Roles.admins, Roles.owners} ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal} +def make_username(reported_by: discord.Member, actioned_by: discord.Member, max_length: int = 80) -> str: + """ + Create a webhook-friendly username from the names of `reported_by` and `actioned_by`. + + If the resulting username length exceeds `max_length`, it will be capped at `max_length - 3` + and have 3 dots appended to the end. The default value is 80, which corresponds to the limit + Discord imposes on webhook username length. + + If the value of `max_length` is < 3, ValueError is raised. + """ + if max_length < 3: + raise ValueError(f"Maximum length cannot be less than 3: {max_length=}") + + username = f"{reported_by.name} | {actioned_by.name}" + log.trace(f"Generated webhook username: {username} (length: {len(username)})") + + if len(username) > max_length: + stop = max_length - 3 + username = f"{username[:stop]}..." + log.trace(f"Username capped at {max_length=}: {username}") + + return username + + def is_incident(message: discord.Message) -> bool: """True if `message` qualifies as an incident, False otherwise.""" conditions = ( diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 2fc9180cf..5700a5a35 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -68,6 +68,35 @@ mock_404 = discord.NotFound( ) +class TestMakeUsername(unittest.TestCase): + """Collection of tests for the `make_username` helper function.""" + + def test_make_username_raises(self): + """Raises `ValueError` on `max_length` < 3.""" + with self.assertRaises(ValueError): + incidents.make_username(MockMember(), MockMember(), max_length=2) + + def test_make_username_never_exceed_limit(self): + """ + The return string length is always less than or equal to `max_length`. + + For this test we pass `max_length=10` for convenience. The name of the first + user (`reported_by`) is always 1 character in length, but we generate names + for the `actioned_by` user starting at length 1 and up to length 20. + + Finally, we assert that the output length never exceeded 10 in total. + """ + user_a = MockMember(name="A") + + max_length = 10 + test_cases = (MockMember(name="B" * n) for n in range(1, 20)) + + for user_b in test_cases: + with self.subTest(user_a=user_a, user_b=user_b, max_length=max_length): + generated_username = incidents.make_username(user_a, user_b, max_length) + self.assertLessEqual(len(generated_username), max_length) + + @patch("bot.constants.Channels.incidents", 123) class TestIsIncident(unittest.TestCase): """ -- cgit v1.2.3 From a8d179d9b04f54b20c5e870bcfa85c78c42c8dca Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sun, 21 Jun 2020 14:21:18 +0200 Subject: Incidents: append `actioned_by` to webhook username Incident author and the moderator who actioned report are now passed through `make_username` to create the webhook username. Tests adjusted as appropriate. --- bot/cogs/moderation/incidents.py | 9 +++++---- tests/bot/cogs/moderation/test_incidents.py | 23 +++++++++++++++++------ 2 files changed, 22 insertions(+), 10 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 2cce9b6fe..72cc4b26c 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -172,13 +172,14 @@ class Incidents(Cog): log.debug("Crawl task finished!") - async def archive(self, incident: discord.Message, outcome: Signal) -> bool: + async def archive(self, incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> bool: """ Relay `incident` to the #incidents-archive channel. The following pieces of information are relayed: * Incident message content (clean, pingless) - * Incident author name (as webhook author) + * Incident author name (as webhook username) + * Name of user who actioned the incident (appended to webhook username) * Incident author avatar (as webhook avatar) * Resolution signal (`outcome`) @@ -194,7 +195,7 @@ class Incidents(Cog): # Now relay the incident message: discord.Message = await webhook.send( content=incident.clean_content, # Clean content will prevent mentions from pinging - username=sub_clyde(incident.author.name), + username=sub_clyde(make_username(incident.author, actioned_by)), avatar_url=incident.author.avatar_url, wait=True, # This makes the method return the sent Message object ) @@ -259,7 +260,7 @@ class Incidents(Cog): log.debug("Reaction was valid, but no action is currently defined for it") return - relay_successful = await self.archive(incident, signal) + relay_successful = await self.archive(incident, signal, actioned_by=member) if not relay_successful: log.trace("Original message will not be deleted as we failed to relay it to the archive") return diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 5700a5a35..a811868e5 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -307,7 +307,9 @@ class TestArchive(TestIncidents): propagate out of the method, which is just as important. """ self.cog_instance.bot.fetch_webhook = AsyncMock(side_effect=mock_404) - self.assertFalse(await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock())) + + result = await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock(), actioned_by=MockMember()) + self.assertFalse(result) async def test_archive_relays_incident(self): """ @@ -332,12 +334,18 @@ class TestArchive(TestIncidents): author=MockUser(name="author_name", avatar_url="author_avatar"), id=123, ) - archive_return = await self.cog_instance.archive(incident, outcome=MagicMock(value="A")) + + with patch("bot.cogs.moderation.incidents.make_username", MagicMock(return_value="generated_username")): + archive_return = await self.cog_instance.archive( + incident=incident, + outcome=MagicMock(value="A"), + actioned_by=MockMember(name="moderator"), + ) # Check that the webhook was dispatched correctly webhook.send.assert_called_once_with( content="pingless message", - username="author_name", + username="generated_username", avatar_url="author_avatar", wait=True, ) @@ -354,7 +362,8 @@ class TestArchive(TestIncidents): Discord will reject any webhook with "clyde" in the username field, as it impersonates the official Clyde bot. Since we do not control what the username will be (the incident - author name is used), we must ensure the name is cleansed, otherwise the relay may fail. + author name, and actioning moderator names are used), we must ensure the name is cleansed, + otherwise the relay may fail. This test assumes the username is passed as a kwarg. If this test fails, please review whether the passed argument is being retrieved correctly. @@ -362,9 +371,11 @@ class TestArchive(TestIncidents): webhook = MockAsyncWebhook() self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) - message_from_clyde = MockMessage(author=MockUser(name="clyde the great")) - await self.cog_instance.archive(message_from_clyde, MagicMock(incidents.Signal)) + # The `make_username` helper will return a string with "clyde" in it + with patch("bot.cogs.moderation.incidents.make_username", MagicMock(return_value="clyde the great")): + await self.cog_instance.archive(MockMessage(), MagicMock(incidents.Signal), MockMember()) + # Assert that the "clyde" was never passed to `send` self.assertNotIn("clyde", webhook.send.call_args.kwargs["username"]) -- cgit v1.2.3 From 195e0f9407d2a8b7ac5b3028b4f10c1b73af0a4f Mon Sep 17 00:00:00 2001 From: Kyle Stanley Date: Fri, 26 Jun 2020 02:08:48 -0400 Subject: Update LinePaginator.add_line() tests --- tests/bot/test_pagination.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'tests') diff --git a/tests/bot/test_pagination.py b/tests/bot/test_pagination.py index f2e2c27ce..74896f010 100644 --- a/tests/bot/test_pagination.py +++ b/tests/bot/test_pagination.py @@ -18,18 +18,18 @@ class LinePaginatorTests(TestCase): self.assertEqual(len(self.paginator._pages), 0) def test_add_line_works_on_long_lines(self): - """`add_line` should scale long lines up to `scale_to_size`.""" - self.paginator.add_line('x' * self.paginator.scale_to_size) - self.assertEqual(len(self.paginator._pages), 1) + """After additional lines after `max_size` is exceeded should go on the next page.""" + self.paginator.add_line('x' * self.paginator.max_size) + self.assertEqual(len(self.paginator._pages), 0) # Any additional lines should start a new page after `max_size` is exceeded. self.paginator.add_line('x') - self.assertEqual(len(self.paginator._pages), 2) + self.assertEqual(len(self.paginator._pages), 1) def test_add_line_continuation(self): """When `scale_to_size` is exceeded, remaining words should be split onto the next page.""" self.paginator.add_line('zyz ' * (self.paginator.scale_to_size//4 + 1)) - self.assertEqual(len(self.paginator._pages), 2) + self.assertEqual(len(self.paginator._pages), 1) def test_add_line_no_continuation(self): """If adding a new line to an existing page would exceed `max_size`, it should start a new -- cgit v1.2.3 From 77ce4c88695ca748059a7076de88d5b42b37d5f5 Mon Sep 17 00:00:00 2001 From: Kyle Stanley Date: Fri, 26 Jun 2020 03:22:30 -0400 Subject: In LinePaginator, truncate words that exceed scale_to_size --- bot/pagination.py | 11 ++++++----- tests/bot/test_pagination.py | 12 +++++------- 2 files changed, 11 insertions(+), 12 deletions(-) (limited to 'tests') diff --git a/bot/pagination.py b/bot/pagination.py index 746ec3696..cd602c715 100644 --- a/bot/pagination.py +++ b/bot/pagination.py @@ -88,11 +88,9 @@ class LinePaginator(Paginator): if len(line) > (max_chars := self.max_size - len(self.prefix) - 2): if len(line) > self.scale_to_size: line, remaining_words = self._split_remaining_words(line, max_chars) - # If line still exceeds scale_to_size, we were unable to split into a second - # page without truncating. if len(line) > self.scale_to_size: - raise RuntimeError(f'Line exceeds maximum scale_to_size {self.scale_to_size}' - ' and could not be split.') + log.debug("Could not continue to next page, truncating line.") + line = line[:self.scale_to_size] if self.max_lines is not None and self._linecount >= self.max_lines: log.debug("max_lines exceeded, creating new page.") @@ -144,11 +142,14 @@ class LinePaginator(Paginator): reduced_words.append(word) reduced_char_count += len(word) + 1 else: + # If reduced_words is empty, we were unable to split the words across pages + if not reduced_words: + return line, None is_full = True remaining_words.append(word) else: remaining_words.append(word) - + return ( " ".join(reduced_words), continuation_header + " ".join(remaining_words) if remaining_words else None diff --git a/tests/bot/test_pagination.py b/tests/bot/test_pagination.py index 74896f010..ce880d457 100644 --- a/tests/bot/test_pagination.py +++ b/tests/bot/test_pagination.py @@ -39,13 +39,11 @@ class LinePaginatorTests(TestCase): self.paginator.add_line('z') self.assertEqual(len(self.paginator._pages), 1) - def test_add_line_raises_on_very_long_words(self): - """`add_line` should raise if a single long word is added that exceeds `scale_to_size`. - - Note: truncation is also a potential option, but this should not occur from normal usage. - """ - with self.assertRaises(RuntimeError): - self.paginator.add_line('x' * (self.paginator.scale_to_size + 1)) + def test_add_line_truncates_very_long_words(self): + """`add_line` should truncate if a single long word exceeds `scale_to_size`.""" + self.paginator.add_line('x' * (self.paginator.scale_to_size + 1)) + # Note: item at index 1 is the truncated line, index 0 is prefix + self.assertEqual(self.paginator._current_page[1], 'x' * self.paginator.scale_to_size) class ImagePaginatorTests(TestCase): -- cgit v1.2.3 From be4a61fb70c485262d36ca2aabf992f3118abcff Mon Sep 17 00:00:00 2001 From: kwzrd Date: Tue, 30 Jun 2020 23:09:00 +0200 Subject: Incidents: revert latest 2 commits Decision was made to use embeds to archive incidents instead of webhooking the raw message. As such, we're reverting the branch to a state from which the adjustments will be easier to make. Reverted commits: * a8d179d9b04f54b20c5e870bcfa85c78c42c8dca * 6fa8caed037b247a7c194f58a4635de7dae21fd2 --- bot/cogs/moderation/incidents.py | 33 +++--------------- tests/bot/cogs/moderation/test_incidents.py | 52 ++++------------------------- 2 files changed, 10 insertions(+), 75 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 72cc4b26c..040f2c0c8 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -41,30 +41,6 @@ ALLOWED_ROLES: t.Set[int] = {Roles.moderators, Roles.admins, Roles.owners} ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal} -def make_username(reported_by: discord.Member, actioned_by: discord.Member, max_length: int = 80) -> str: - """ - Create a webhook-friendly username from the names of `reported_by` and `actioned_by`. - - If the resulting username length exceeds `max_length`, it will be capped at `max_length - 3` - and have 3 dots appended to the end. The default value is 80, which corresponds to the limit - Discord imposes on webhook username length. - - If the value of `max_length` is < 3, ValueError is raised. - """ - if max_length < 3: - raise ValueError(f"Maximum length cannot be less than 3: {max_length=}") - - username = f"{reported_by.name} | {actioned_by.name}" - log.trace(f"Generated webhook username: {username} (length: {len(username)})") - - if len(username) > max_length: - stop = max_length - 3 - username = f"{username[:stop]}..." - log.trace(f"Username capped at {max_length=}: {username}") - - return username - - def is_incident(message: discord.Message) -> bool: """True if `message` qualifies as an incident, False otherwise.""" conditions = ( @@ -172,14 +148,13 @@ class Incidents(Cog): log.debug("Crawl task finished!") - async def archive(self, incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> bool: + async def archive(self, incident: discord.Message, outcome: Signal) -> bool: """ Relay `incident` to the #incidents-archive channel. The following pieces of information are relayed: * Incident message content (clean, pingless) - * Incident author name (as webhook username) - * Name of user who actioned the incident (appended to webhook username) + * Incident author name (as webhook author) * Incident author avatar (as webhook avatar) * Resolution signal (`outcome`) @@ -195,7 +170,7 @@ class Incidents(Cog): # Now relay the incident message: discord.Message = await webhook.send( content=incident.clean_content, # Clean content will prevent mentions from pinging - username=sub_clyde(make_username(incident.author, actioned_by)), + username=sub_clyde(incident.author.name), avatar_url=incident.author.avatar_url, wait=True, # This makes the method return the sent Message object ) @@ -260,7 +235,7 @@ class Incidents(Cog): log.debug("Reaction was valid, but no action is currently defined for it") return - relay_successful = await self.archive(incident, signal, actioned_by=member) + relay_successful = await self.archive(incident, signal) if not relay_successful: log.trace("Original message will not be deleted as we failed to relay it to the archive") return diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index a811868e5..2fc9180cf 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -68,35 +68,6 @@ mock_404 = discord.NotFound( ) -class TestMakeUsername(unittest.TestCase): - """Collection of tests for the `make_username` helper function.""" - - def test_make_username_raises(self): - """Raises `ValueError` on `max_length` < 3.""" - with self.assertRaises(ValueError): - incidents.make_username(MockMember(), MockMember(), max_length=2) - - def test_make_username_never_exceed_limit(self): - """ - The return string length is always less than or equal to `max_length`. - - For this test we pass `max_length=10` for convenience. The name of the first - user (`reported_by`) is always 1 character in length, but we generate names - for the `actioned_by` user starting at length 1 and up to length 20. - - Finally, we assert that the output length never exceeded 10 in total. - """ - user_a = MockMember(name="A") - - max_length = 10 - test_cases = (MockMember(name="B" * n) for n in range(1, 20)) - - for user_b in test_cases: - with self.subTest(user_a=user_a, user_b=user_b, max_length=max_length): - generated_username = incidents.make_username(user_a, user_b, max_length) - self.assertLessEqual(len(generated_username), max_length) - - @patch("bot.constants.Channels.incidents", 123) class TestIsIncident(unittest.TestCase): """ @@ -307,9 +278,7 @@ class TestArchive(TestIncidents): propagate out of the method, which is just as important. """ self.cog_instance.bot.fetch_webhook = AsyncMock(side_effect=mock_404) - - result = await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock(), actioned_by=MockMember()) - self.assertFalse(result) + self.assertFalse(await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock())) async def test_archive_relays_incident(self): """ @@ -334,18 +303,12 @@ class TestArchive(TestIncidents): author=MockUser(name="author_name", avatar_url="author_avatar"), id=123, ) - - with patch("bot.cogs.moderation.incidents.make_username", MagicMock(return_value="generated_username")): - archive_return = await self.cog_instance.archive( - incident=incident, - outcome=MagicMock(value="A"), - actioned_by=MockMember(name="moderator"), - ) + archive_return = await self.cog_instance.archive(incident, outcome=MagicMock(value="A")) # Check that the webhook was dispatched correctly webhook.send.assert_called_once_with( content="pingless message", - username="generated_username", + username="author_name", avatar_url="author_avatar", wait=True, ) @@ -362,8 +325,7 @@ class TestArchive(TestIncidents): Discord will reject any webhook with "clyde" in the username field, as it impersonates the official Clyde bot. Since we do not control what the username will be (the incident - author name, and actioning moderator names are used), we must ensure the name is cleansed, - otherwise the relay may fail. + author name is used), we must ensure the name is cleansed, otherwise the relay may fail. This test assumes the username is passed as a kwarg. If this test fails, please review whether the passed argument is being retrieved correctly. @@ -371,11 +333,9 @@ class TestArchive(TestIncidents): webhook = MockAsyncWebhook() self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) - # The `make_username` helper will return a string with "clyde" in it - with patch("bot.cogs.moderation.incidents.make_username", MagicMock(return_value="clyde the great")): - await self.cog_instance.archive(MockMessage(), MagicMock(incidents.Signal), MockMember()) + message_from_clyde = MockMessage(author=MockUser(name="clyde the great")) + await self.cog_instance.archive(message_from_clyde, MagicMock(incidents.Signal)) - # Assert that the "clyde" was never passed to `send` self.assertNotIn("clyde", webhook.send.call_args.kwargs["username"]) -- cgit v1.2.3 From 968251660768297383401576902a71f8ac9edada Mon Sep 17 00:00:00 2001 From: kwzrd Date: Tue, 30 Jun 2020 23:15:02 +0200 Subject: Incidents: pass `actioned_by` to `archive` This is an important piece of information that shall be relayed. --- bot/cogs/moderation/incidents.py | 4 ++-- tests/bot/cogs/moderation/test_incidents.py | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 040f2c0c8..580a258fe 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -148,7 +148,7 @@ class Incidents(Cog): log.debug("Crawl task finished!") - async def archive(self, incident: discord.Message, outcome: Signal) -> bool: + async def archive(self, incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> bool: """ Relay `incident` to the #incidents-archive channel. @@ -235,7 +235,7 @@ class Incidents(Cog): log.debug("Reaction was valid, but no action is currently defined for it") return - relay_successful = await self.archive(incident, signal) + relay_successful = await self.archive(incident, signal, actioned_by=member) if not relay_successful: log.trace("Original message will not be deleted as we failed to relay it to the archive") return diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 2fc9180cf..c2e32fe6b 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -278,7 +278,9 @@ class TestArchive(TestIncidents): propagate out of the method, which is just as important. """ self.cog_instance.bot.fetch_webhook = AsyncMock(side_effect=mock_404) - self.assertFalse(await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock())) + self.assertFalse( + await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock(), actioned_by=MockMember()) + ) async def test_archive_relays_incident(self): """ @@ -303,7 +305,7 @@ class TestArchive(TestIncidents): author=MockUser(name="author_name", avatar_url="author_avatar"), id=123, ) - archive_return = await self.cog_instance.archive(incident, outcome=MagicMock(value="A")) + archive_return = await self.cog_instance.archive(incident, MagicMock(value="A"), MockMember()) # Check that the webhook was dispatched correctly webhook.send.assert_called_once_with( @@ -334,7 +336,7 @@ class TestArchive(TestIncidents): self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) message_from_clyde = MockMessage(author=MockUser(name="clyde the great")) - await self.cog_instance.archive(message_from_clyde, MagicMock(incidents.Signal)) + await self.cog_instance.archive(message_from_clyde, MagicMock(incidents.Signal), MockMember()) self.assertNotIn("clyde", webhook.send.call_args.kwargs["username"]) -- cgit v1.2.3 From dd74105d4a4433bb9e9e6fa57960a4956c0f1231 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Tue, 30 Jun 2020 23:42:32 +0200 Subject: Incidents: implement `make_embed` helper & tests See `make_embed` docstring for further information. The tests are fairly loose and should be easily adjustable in the future should changes be made. --- bot/cogs/moderation/incidents.py | 32 ++++++++++++++++++++++++++++- tests/bot/cogs/moderation/test_incidents.py | 26 +++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 580a258fe..ca591fc6e 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -1,13 +1,14 @@ import asyncio import logging import typing as t +from datetime import datetime from enum import Enum import discord from discord.ext.commands import Cog from bot.bot import Bot -from bot.constants import Channels, Emojis, Roles, Webhooks +from bot.constants import Channels, Colours, Emojis, Roles, Webhooks from bot.utils.messages import sub_clyde log = logging.getLogger(__name__) @@ -41,6 +42,35 @@ ALLOWED_ROLES: t.Set[int] = {Roles.moderators, Roles.admins, Roles.owners} ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal} +def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> discord.Embed: + """ + Create an embed representation of `incident` for the #incidents-archive channel. + + The name & discriminator of `actioned_by` and `outcome` will be presented in the + embed footer. Additionally, the embed is coloured based on `outcome`. + + The author of `incident` is not shown in the embed. It is assumed that this piece + of information will be relayed in other ways, e.g. webhook username. + + As mentions in embeds do not ping, we do not need to use `incident.clean_content`. + """ + if outcome is Signal.ACTIONED: + colour = Colours.soft_green + footer = f"Actioned by {actioned_by}" + else: + colour = Colours.soft_red + footer = f"Rejected by {actioned_by}" + + embed = discord.Embed( + description=incident.content, + timestamp=datetime.utcnow(), + colour=colour, + ) + embed.set_footer(text=footer, icon_url=actioned_by.avatar_url) + + return embed + + def is_incident(message: discord.Message) -> bool: """True if `message` qualifies as an incident, False otherwise.""" conditions = ( diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index c2e32fe6b..4731a786d 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -9,6 +9,7 @@ import aiohttp import discord from bot.cogs.moderation import Incidents, incidents +from bot.constants import Colours from tests.helpers import ( MockAsyncWebhook, MockBot, @@ -68,6 +69,31 @@ mock_404 = discord.NotFound( ) +class TestMakeEmbed(unittest.TestCase): + """Collection of tests for the `make_embed` helper function.""" + + def test_make_embed_actioned(self): + """Embed is coloured green and footer contains 'Actioned' when `outcome=Signal.ACTIONED`.""" + embed = incidents.make_embed(MockMessage(), incidents.Signal.ACTIONED, MockMember()) + + self.assertEqual(embed.colour.value, Colours.soft_green) + self.assertIn("Actioned", embed.footer.text) + + def test_make_embed_not_actioned(self): + """Embed is coloured red and footer contains 'Rejected' when `outcome=Signal.NOT_ACTIONED`.""" + embed = incidents.make_embed(MockMessage(), incidents.Signal.NOT_ACTIONED, MockMember()) + + self.assertEqual(embed.colour.value, Colours.soft_red) + self.assertIn("Rejected", embed.footer.text) + + def test_make_embed_content(self): + """Incident content appears as embed description.""" + incident = MockMessage(content="this is an incident") + embed = incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember()) + + self.assertEqual(incident.content, embed.description) + + @patch("bot.constants.Channels.incidents", 123) class TestIsIncident(unittest.TestCase): """ -- cgit v1.2.3 From 744aed585162cb0547e61a538734f116459ab510 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Wed, 1 Jul 2020 16:52:58 +0200 Subject: Incidents: relay incidents as embeds rather than raw content This applies the previously defined `make_embed` function. As the `archive` function is now simpler, I decided to reduce the amount of whitespace ~ it's a lot more compact now. Tests are adjusted as appropriate. --- bot/cogs/moderation/incidents.py | 24 ++++++++-------------- tests/bot/cogs/moderation/test_incidents.py | 32 ++++++++++------------------- 2 files changed, 19 insertions(+), 37 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index ca591fc6e..3a1a3d84e 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -180,38 +180,30 @@ class Incidents(Cog): async def archive(self, incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> bool: """ - Relay `incident` to the #incidents-archive channel. + Relay an embed representation of `incident` to the #incidents-archive channel. The following pieces of information are relayed: - * Incident message content (clean, pingless) + * Incident message content (as embed description) * Incident author name (as webhook author) * Incident author avatar (as webhook avatar) - * Resolution signal (`outcome`) + * Resolution signal `outcome` (as embed colour & footer) + * Moderator `actioned_by` (name & discriminator shown in footer) Return True if the relay finishes successfully. If anything goes wrong, meaning not all information was relayed, return False. This signals that the original message is not safe to be deleted, as we will lose some information. """ - log.debug(f"Archiving incident: {incident.id} with outcome: {outcome}") + log.debug(f"Archiving incident: {incident.id} (outcome: {outcome}, actioned by: {actioned_by})") try: - # First we try to grab the webhook - webhook: discord.Webhook = await self.bot.fetch_webhook(Webhooks.incidents_archive) - - # Now relay the incident - message: discord.Message = await webhook.send( - content=incident.clean_content, # Clean content will prevent mentions from pinging + webhook = await self.bot.fetch_webhook(Webhooks.incidents_archive) + await webhook.send( + embed=make_embed(incident, outcome, actioned_by), username=sub_clyde(incident.author.name), avatar_url=incident.author.avatar_url, - wait=True, # This makes the method return the sent Message object ) - - # Finally add the `outcome` emoji - await message.add_reaction(outcome.value) - except Exception: log.exception(f"Failed to archive incident {incident.id} to #incidents-archive") return False - else: log.trace("Message archived successfully!") return True diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 4731a786d..70dfe6b5f 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -312,39 +312,29 @@ class TestArchive(TestIncidents): """ If webhook is found, method relays `incident` properly. - This test will assert the following: - * The fetched webhook's `send` method is fed the correct arguments - * The message returned by `send` will have `outcome` reaction added - * Finally, the `archive` method returns True - - Assertions are made specifically in this order. + This test will assert that the fetched webhook's `send` method is fed the correct arguments, + and that the `archive` method returns True. """ - webhook_message = MockMessage() # The message that will be returned by the webhook's `send` method - webhook = MockAsyncWebhook(send=AsyncMock(return_value=webhook_message)) - + webhook = MockAsyncWebhook() self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) # Patch in our webhook - # Now we'll pas our own `incident` to `archive` and capture the return value + # Define our own `incident` for archivation incident = MockMessage( - clean_content="pingless message", - content="pingful message", + content="this is an incident", author=MockUser(name="author_name", avatar_url="author_avatar"), id=123, ) - archive_return = await self.cog_instance.archive(incident, MagicMock(value="A"), MockMember()) + built_embed = MagicMock(discord.Embed, id=123) # We patch `make_embed` to return this - # Check that the webhook was dispatched correctly + with patch("bot.cogs.moderation.incidents.make_embed", MagicMock(return_value=built_embed)): + archive_return = await self.cog_instance.archive(incident, MagicMock(value="A"), MockMember()) + + # Now we check that the webhook was given the correct args, and that `archive` returned True webhook.send.assert_called_once_with( - content="pingless message", + embed=built_embed, username="author_name", avatar_url="author_avatar", - wait=True, ) - - # Now check that the correct emoji was added to the relayed message - webhook_message.add_reaction.assert_called_once_with("A") - - # Finally check that the method returned True self.assertTrue(archive_return) async def test_archive_clyde_username(self): -- cgit v1.2.3 From 83544ca0f91dd7bc8510e4fc7a64bc73712ddaf8 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 3 Jul 2020 10:47:47 +0200 Subject: Incidents: archive incident attachments There is no handling of file types as explained in the `archive` docstring. Testing indicates that relaying incidents with e.g. a text file attachment is simply a noop in the Discord GUI. If there is at least one attachment, we always only relay the one at index 0, as it is believed the user-sent messages can only contain one attachment at maximum. This also adds an extra test asserting the behaviour when an incident with an attachment is archived. The existing test for `archive` is adjusted to assume no attachments. Joe helped me conceive & test this. Co-authored-by: Joseph Banks --- bot/cogs/moderation/incidents.py | 21 +++++++++++++++++++- tests/bot/cogs/moderation/test_incidents.py | 30 +++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 8970c2c5c..1a12c8bbd 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -186,22 +186,41 @@ class Incidents(Cog): The following pieces of information are relayed: * Incident message content (as embed description) + * Incident attachment (if image, shown in archive embed) * Incident author name (as webhook author) * Incident author avatar (as webhook avatar) * Resolution signal `outcome` (as embed colour & footer) * Moderator `actioned_by` (name & discriminator shown in footer) + If `incident` contains an attachment, we try to add it to the archive embed. There is + no handing of extensions / file types - we simply dispatch the attachment file with the + webhook, and try to display it in the embed. Testing indicates that if the attachment + cannot be displayed (e.g. a text file), it's invisible in the embed, with no error. + Return True if the relay finishes successfully. If anything goes wrong, meaning not all information was relayed, return False. This signals that the original message is not safe to be deleted, as we will lose some information. """ log.debug(f"Archiving incident: {incident.id} (outcome: {outcome}, actioned by: {actioned_by})") + embed = make_embed(incident, outcome, actioned_by) + + # If the incident had an attachment, we will try to relay it + if incident.attachments: + attachment = incident.attachments[0] # User-sent messages can only contain one attachment + log.debug(f"Attempting to archive incident attachment: {attachment.filename}") + + attachment_file = await attachment.to_file() # The file will be sent with the webhook + embed.set_image(url=f"attachment://{attachment.filename}") # Embed displays the attached file + else: + attachment_file = None + try: webhook = await self.bot.fetch_webhook(Webhooks.incidents_archive) await webhook.send( - embed=make_embed(incident, outcome, actioned_by), + embed=embed, username=sub_clyde(incident.author.name), avatar_url=incident.author.avatar_url, + file=attachment_file, ) except Exception: log.exception(f"Failed to archive incident {incident.id} to #incidents-archive") diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 70dfe6b5f..f8d479cef 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -323,6 +323,7 @@ class TestArchive(TestIncidents): content="this is an incident", author=MockUser(name="author_name", avatar_url="author_avatar"), id=123, + attachments=[], # This incident has no attachments ) built_embed = MagicMock(discord.Embed, id=123) # We patch `make_embed` to return this @@ -334,9 +335,38 @@ class TestArchive(TestIncidents): embed=built_embed, username="author_name", avatar_url="author_avatar", + file=None, ) self.assertTrue(archive_return) + async def test_archive_relays_incident_with_attachments(self): + """ + Incident attachments are relayed and displayed in the embed. + + This test asserts the two things that need to happen in order to relay the attachment. + The embed returned by `make_embed` must have the `set_image` method called with the + attachment's filename, and the file must be passed to the webhook's send method. + """ + attachment_file = MagicMock(discord.File) + attachment = MagicMock( + discord.Attachment, + filename="abc.png", + to_file=AsyncMock(return_value=attachment_file), + ) + incident = MockMessage( + attachments=[attachment], + ) + built_embed = MagicMock(discord.Embed) + + with patch("bot.cogs.moderation.incidents.make_embed", MagicMock(return_value=built_embed)): + await self.cog_instance.archive(incident, incidents.Signal.ACTIONED, actioned_by=MockMember()) + + built_embed.set_image.assert_called_once_with(url="attachment://abc.png") + + send_kwargs = self.cog_instance.bot.fetch_webhook.return_value.send.call_args.kwargs + self.assertIn("file", send_kwargs) + self.assertIs(send_kwargs["file"], attachment_file) + async def test_archive_clyde_username(self): """ The archive webhook username is cleansed using `sub_clyde`. -- cgit v1.2.3 From 40719793f9c0d8a2c5761d3730b5920a146709c3 Mon Sep 17 00:00:00 2001 From: Den4200 Date: Mon, 6 Jul 2020 04:08:27 +0000 Subject: Add tests for cog_check and get_slowmode --- tests/bot/cogs/test_slowmode.py | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 tests/bot/cogs/test_slowmode.py (limited to 'tests') diff --git a/tests/bot/cogs/test_slowmode.py b/tests/bot/cogs/test_slowmode.py new file mode 100644 index 000000000..fb9f3c9ad --- /dev/null +++ b/tests/bot/cogs/test_slowmode.py @@ -0,0 +1,37 @@ +import unittest +from unittest import mock + +from bot.cogs.slowmode import Slowmode +from tests.helpers import MockBot, MockContext, MockTextChannel + + +class SlowmodeTests(unittest.IsolatedAsyncioTestCase): + + def setUp(self) -> None: + self.bot = MockBot() + self.cog = Slowmode(self.bot) + self.text_channel = MockTextChannel() + self.ctx = MockContext(channel=self.text_channel) + + async def test_get_slowmode_no_channel(self) -> None: + """Get slowmode without a given channel""" + self.text_channel.mention = '#python-general' + self.text_channel.slowmode_delay = 5 + + await self.cog.get_slowmode(self.cog, self.ctx, None) + self.ctx.send.assert_called_once_with("The slowmode delay for #python-general is 5 seconds.") + + async def test_get_slowmode_with_channel(self) -> None: + """Get slowmode without a given channel""" + self.text_channel.mention = '#python-language' + self.text_channel.slowmode_delay = 2 + + await self.cog.get_slowmode(self.cog, self.ctx, self.text_channel) + self.ctx.send.assert_called_once_with("The slowmode delay for #python-language is 2 seconds.") + + @mock.patch("bot.cogs.slowmode.with_role_check") + @mock.patch("bot.cogs.slowmode.MODERATION_ROLES", new=(1, 2, 3)) + def test_cog_check(self, role_check): + """Role check is called with `MODERATION_ROLES`""" + self.cog.cog_check(self.ctx) + role_check.assert_called_once_with(self.ctx, *(1, 2, 3)) -- cgit v1.2.3 From e760b4312a5264fe9442cb1d53c9e357dbeb2b81 Mon Sep 17 00:00:00 2001 From: Den4200 Date: Mon, 6 Jul 2020 04:55:42 +0000 Subject: Add tests for reset_slowmode --- tests/bot/cogs/test_slowmode.py | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_slowmode.py b/tests/bot/cogs/test_slowmode.py index fb9f3c9ad..a2e5ad346 100644 --- a/tests/bot/cogs/test_slowmode.py +++ b/tests/bot/cogs/test_slowmode.py @@ -2,6 +2,7 @@ import unittest from unittest import mock from bot.cogs.slowmode import Slowmode +from bot.constants import Emojis from tests.helpers import MockBot, MockContext, MockTextChannel @@ -14,7 +15,7 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): self.ctx = MockContext(channel=self.text_channel) async def test_get_slowmode_no_channel(self) -> None: - """Get slowmode without a given channel""" + """Get slowmode without a given channel.""" self.text_channel.mention = '#python-general' self.text_channel.slowmode_delay = 5 @@ -22,12 +23,30 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): self.ctx.send.assert_called_once_with("The slowmode delay for #python-general is 5 seconds.") async def test_get_slowmode_with_channel(self) -> None: - """Get slowmode without a given channel""" + """Get slowmode with a given channel.""" self.text_channel.mention = '#python-language' self.text_channel.slowmode_delay = 2 await self.cog.get_slowmode(self.cog, self.ctx, self.text_channel) - self.ctx.send.assert_called_once_with("The slowmode delay for #python-language is 2 seconds.") + self.ctx.send.assert_called_once_with('The slowmode delay for #python-language is 2 seconds.') + + async def test_reset_slowmode_no_channel(self) -> None: + """Reset slowmode without a given channel.""" + self.text_channel.mention = '#careers' + + await self.cog.reset_slowmode(self.cog, self.ctx, None) + self.ctx.send.assert_called_once_with( + f'{Emojis.check_mark} The slowmode delay for #careers has been reset to 0 seconds.' + ) + + async def test_reset_slowmode_with_channel(self) -> None: + """Reset slowmode with a given channel.""" + self.text_channel.mention = '#meta' + + await self.cog.reset_slowmode(self.cog, self.ctx, self.text_channel) + self.ctx.send.assert_called_once_with( + f'{Emojis.check_mark} The slowmode delay for #meta has been reset to 0 seconds.' + ) @mock.patch("bot.cogs.slowmode.with_role_check") @mock.patch("bot.cogs.slowmode.MODERATION_ROLES", new=(1, 2, 3)) -- cgit v1.2.3 From 8613659cb191bedca925dc798c89623b49c9a90a Mon Sep 17 00:00:00 2001 From: Den4200 Date: Mon, 6 Jul 2020 05:45:04 +0000 Subject: Add tests for set_slowmode --- tests/bot/cogs/test_slowmode.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_slowmode.py b/tests/bot/cogs/test_slowmode.py index a2e5ad346..5262ce34a 100644 --- a/tests/bot/cogs/test_slowmode.py +++ b/tests/bot/cogs/test_slowmode.py @@ -1,6 +1,8 @@ import unittest from unittest import mock +from dateutil.relativedelta import relativedelta + from bot.cogs.slowmode import Slowmode from bot.constants import Emojis from tests.helpers import MockBot, MockContext, MockTextChannel @@ -30,6 +32,24 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): await self.cog.get_slowmode(self.cog, self.ctx, self.text_channel) self.ctx.send.assert_called_once_with('The slowmode delay for #python-language is 2 seconds.') + async def test_set_slowmode_no_channel(self) -> None: + """Set slowmode without a given channel.""" + self.text_channel.mention = '#careers' + + await self.cog.set_slowmode(self.cog, self.ctx, None, relativedelta(seconds=3)) + self.ctx.send.assert_called_once_with( + f'{Emojis.check_mark} The slowmode delay for #careers is now 3 seconds.' + ) + + async def test_set_slowmode_with_channel(self) -> None: + """Set slowmode with a given channel.""" + self.text_channel.mention = '#meta' + + await self.cog.set_slowmode(self.cog, self.ctx, self.text_channel, relativedelta(seconds=4)) + self.ctx.send.assert_called_once_with( + f'{Emojis.check_mark} The slowmode delay for #meta is now 4 seconds.' + ) + async def test_reset_slowmode_no_channel(self) -> None: """Reset slowmode without a given channel.""" self.text_channel.mention = '#careers' -- cgit v1.2.3 From 4935ed5ae632f5887bcff23ac67c781eab8527e9 Mon Sep 17 00:00:00 2001 From: Den4200 Date: Mon, 6 Jul 2020 06:05:32 +0000 Subject: Use local text_channel instead of instance attribute --- tests/bot/cogs/test_slowmode.py | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_slowmode.py b/tests/bot/cogs/test_slowmode.py index 5262ce34a..663c9fd43 100644 --- a/tests/bot/cogs/test_slowmode.py +++ b/tests/bot/cogs/test_slowmode.py @@ -13,28 +13,25 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): def setUp(self) -> None: self.bot = MockBot() self.cog = Slowmode(self.bot) - self.text_channel = MockTextChannel() - self.ctx = MockContext(channel=self.text_channel) + self.ctx = MockContext() async def test_get_slowmode_no_channel(self) -> None: """Get slowmode without a given channel.""" - self.text_channel.mention = '#python-general' - self.text_channel.slowmode_delay = 5 + self.ctx.channel = MockTextChannel(name='python-general', slowmode_delay=5) await self.cog.get_slowmode(self.cog, self.ctx, None) self.ctx.send.assert_called_once_with("The slowmode delay for #python-general is 5 seconds.") async def test_get_slowmode_with_channel(self) -> None: """Get slowmode with a given channel.""" - self.text_channel.mention = '#python-language' - self.text_channel.slowmode_delay = 2 + text_channel = MockTextChannel(name='python-language', slowmode_delay=2) - await self.cog.get_slowmode(self.cog, self.ctx, self.text_channel) + await self.cog.get_slowmode(self.cog, self.ctx, text_channel) self.ctx.send.assert_called_once_with('The slowmode delay for #python-language is 2 seconds.') async def test_set_slowmode_no_channel(self) -> None: """Set slowmode without a given channel.""" - self.text_channel.mention = '#careers' + self.ctx.channel = MockTextChannel(name='careers') await self.cog.set_slowmode(self.cog, self.ctx, None, relativedelta(seconds=3)) self.ctx.send.assert_called_once_with( @@ -43,16 +40,16 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): async def test_set_slowmode_with_channel(self) -> None: """Set slowmode with a given channel.""" - self.text_channel.mention = '#meta' + text_channel = MockTextChannel(name='meta') - await self.cog.set_slowmode(self.cog, self.ctx, self.text_channel, relativedelta(seconds=4)) + await self.cog.set_slowmode(self.cog, self.ctx, text_channel, relativedelta(seconds=4)) self.ctx.send.assert_called_once_with( f'{Emojis.check_mark} The slowmode delay for #meta is now 4 seconds.' ) async def test_reset_slowmode_no_channel(self) -> None: """Reset slowmode without a given channel.""" - self.text_channel.mention = '#careers' + self.ctx.channel = MockTextChannel(name='careers', slowmode_delay=6) await self.cog.reset_slowmode(self.cog, self.ctx, None) self.ctx.send.assert_called_once_with( @@ -61,9 +58,9 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): async def test_reset_slowmode_with_channel(self) -> None: """Reset slowmode with a given channel.""" - self.text_channel.mention = '#meta' + text_channel = MockTextChannel(name='meta', slowmode_delay=1) - await self.cog.reset_slowmode(self.cog, self.ctx, self.text_channel) + await self.cog.reset_slowmode(self.cog, self.ctx, text_channel) self.ctx.send.assert_called_once_with( f'{Emojis.check_mark} The slowmode delay for #meta has been reset to 0 seconds.' ) -- cgit v1.2.3 From 77a2e514dd2e200e23ccf45760677c2e7c40b9ff Mon Sep 17 00:00:00 2001 From: Den4200 Date: Mon, 6 Jul 2020 06:11:00 +0000 Subject: Add multiple test cases for set_slowmode tests --- tests/bot/cogs/test_slowmode.py | 44 +++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_slowmode.py b/tests/bot/cogs/test_slowmode.py index 663c9fd43..e9835b8bd 100644 --- a/tests/bot/cogs/test_slowmode.py +++ b/tests/bot/cogs/test_slowmode.py @@ -31,22 +31,46 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): async def test_set_slowmode_no_channel(self) -> None: """Set slowmode without a given channel.""" - self.ctx.channel = MockTextChannel(name='careers') - - await self.cog.set_slowmode(self.cog, self.ctx, None, relativedelta(seconds=3)) - self.ctx.send.assert_called_once_with( - f'{Emojis.check_mark} The slowmode delay for #careers is now 3 seconds.' + test_cases = ( + ('helpers', 23, f'{Emojis.check_mark} The slowmode delay for #helpers is now 23 seconds.'), + ('mods', 76526, f'{Emojis.cross_mark} The slowmode delay must be between 0 and 6 hours.'), + ('admins', 97, f'{Emojis.check_mark} The slowmode delay for #admins is now 1 minute and 37 seconds.') ) + for channel_name, seconds, result_msg in test_cases: + with self.subTest( + channel_mention=channel_name, + seconds=seconds, + result_msg=result_msg + ): + self.ctx.channel = MockTextChannel(name=channel_name) + + await self.cog.set_slowmode(self.cog, self.ctx, None, relativedelta(seconds=seconds)) + self.ctx.send.assert_called_once_with(result_msg) + + self.ctx.reset_mock() + async def test_set_slowmode_with_channel(self) -> None: """Set slowmode with a given channel.""" - text_channel = MockTextChannel(name='meta') - - await self.cog.set_slowmode(self.cog, self.ctx, text_channel, relativedelta(seconds=4)) - self.ctx.send.assert_called_once_with( - f'{Emojis.check_mark} The slowmode delay for #meta is now 4 seconds.' + test_cases = ( + ('bot-commands', 12, f'{Emojis.check_mark} The slowmode delay for #bot-commands is now 12 seconds.'), + ('mod-spam', 21, f'{Emojis.check_mark} The slowmode delay for #mod-spam is now 21 seconds.'), + ('admin-spam', 4323598, f'{Emojis.cross_mark} The slowmode delay must be between 0 and 6 hours.') ) + for channel_name, seconds, result_msg in test_cases: + with self.subTest( + channel_mention=channel_name, + seconds=seconds, + result_msg=result_msg + ): + text_channel = MockTextChannel(name=channel_name) + + await self.cog.set_slowmode(self.cog, self.ctx, text_channel, relativedelta(seconds=seconds)) + self.ctx.send.assert_called_once_with(result_msg) + + self.ctx.reset_mock() + async def test_reset_slowmode_no_channel(self) -> None: """Reset slowmode without a given channel.""" self.ctx.channel = MockTextChannel(name='careers', slowmode_delay=6) -- cgit v1.2.3 From 2d170b8af92c77bedea4d77fbdeedc515d3f2c59 Mon Sep 17 00:00:00 2001 From: Den4200 Date: Mon, 6 Jul 2020 17:08:24 +0000 Subject: Improve set_slowmode tests by checking whether the channel was edited --- tests/bot/cogs/test_slowmode.py | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_slowmode.py b/tests/bot/cogs/test_slowmode.py index e9835b8bd..65b1534cb 100644 --- a/tests/bot/cogs/test_slowmode.py +++ b/tests/bot/cogs/test_slowmode.py @@ -32,20 +32,27 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): async def test_set_slowmode_no_channel(self) -> None: """Set slowmode without a given channel.""" test_cases = ( - ('helpers', 23, f'{Emojis.check_mark} The slowmode delay for #helpers is now 23 seconds.'), - ('mods', 76526, f'{Emojis.cross_mark} The slowmode delay must be between 0 and 6 hours.'), - ('admins', 97, f'{Emojis.check_mark} The slowmode delay for #admins is now 1 minute and 37 seconds.') + ('helpers', 23, True, f'{Emojis.check_mark} The slowmode delay for #helpers is now 23 seconds.'), + ('mods', 76526, False, f'{Emojis.cross_mark} The slowmode delay must be between 0 and 6 hours.'), + ('admins', 97, True, f'{Emojis.check_mark} The slowmode delay for #admins is now 1 minute and 37 seconds.') ) - for channel_name, seconds, result_msg in test_cases: + for channel_name, seconds, edited, result_msg in test_cases: with self.subTest( channel_mention=channel_name, seconds=seconds, + edited=edited, result_msg=result_msg ): self.ctx.channel = MockTextChannel(name=channel_name) await self.cog.set_slowmode(self.cog, self.ctx, None, relativedelta(seconds=seconds)) + + if edited: + self.ctx.channel.edit.assert_awaited_once_with(slowmode_delay=float(seconds)) + else: + self.ctx.channel.edit.assert_not_called() + self.ctx.send.assert_called_once_with(result_msg) self.ctx.reset_mock() @@ -53,20 +60,27 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): async def test_set_slowmode_with_channel(self) -> None: """Set slowmode with a given channel.""" test_cases = ( - ('bot-commands', 12, f'{Emojis.check_mark} The slowmode delay for #bot-commands is now 12 seconds.'), - ('mod-spam', 21, f'{Emojis.check_mark} The slowmode delay for #mod-spam is now 21 seconds.'), - ('admin-spam', 4323598, f'{Emojis.cross_mark} The slowmode delay must be between 0 and 6 hours.') + ('bot-commands', 12, True, f'{Emojis.check_mark} The slowmode delay for #bot-commands is now 12 seconds.'), + ('mod-spam', 21, True, f'{Emojis.check_mark} The slowmode delay for #mod-spam is now 21 seconds.'), + ('admin-spam', 4323598, False, f'{Emojis.cross_mark} The slowmode delay must be between 0 and 6 hours.') ) - for channel_name, seconds, result_msg in test_cases: + for channel_name, seconds, edited, result_msg in test_cases: with self.subTest( channel_mention=channel_name, seconds=seconds, + edited=edited, result_msg=result_msg ): text_channel = MockTextChannel(name=channel_name) await self.cog.set_slowmode(self.cog, self.ctx, text_channel, relativedelta(seconds=seconds)) + + if edited: + text_channel.edit.assert_awaited_once_with(slowmode_delay=float(seconds)) + else: + text_channel.edit.assert_not_called() + self.ctx.send.assert_called_once_with(result_msg) self.ctx.reset_mock() -- cgit v1.2.3 From cdeb41bfd283cb6cb1285993737e8e3abd5aea9f Mon Sep 17 00:00:00 2001 From: Den4200 Date: Mon, 6 Jul 2020 17:30:44 +0000 Subject: Fix imports in slowmode tests --- tests/bot/cogs/test_slowmode.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_slowmode.py b/tests/bot/cogs/test_slowmode.py index 65b1534cb..f442814c8 100644 --- a/tests/bot/cogs/test_slowmode.py +++ b/tests/bot/cogs/test_slowmode.py @@ -3,7 +3,7 @@ from unittest import mock from dateutil.relativedelta import relativedelta -from bot.cogs.slowmode import Slowmode +from bot.cogs.moderation.slowmode import Slowmode from bot.constants import Emojis from tests.helpers import MockBot, MockContext, MockTextChannel @@ -103,8 +103,8 @@ class SlowmodeTests(unittest.IsolatedAsyncioTestCase): f'{Emojis.check_mark} The slowmode delay for #meta has been reset to 0 seconds.' ) - @mock.patch("bot.cogs.slowmode.with_role_check") - @mock.patch("bot.cogs.slowmode.MODERATION_ROLES", new=(1, 2, 3)) + @mock.patch("bot.cogs.moderation.slowmode.with_role_check") + @mock.patch("bot.cogs.moderation.slowmode.MODERATION_ROLES", new=(1, 2, 3)) def test_cog_check(self, role_check): """Role check is called with `MODERATION_ROLES`""" self.cog.cog_check(self.ctx) -- cgit v1.2.3 From ddb1f556ace346a97b8639f278fae8915078e78d Mon Sep 17 00:00:00 2001 From: kwzrd Date: Thu, 9 Jul 2020 12:11:16 +0200 Subject: Incidents tests: improve in-line comment wording Co-authored-by: MarkKoz --- tests/bot/cogs/moderation/test_incidents.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index f8d479cef..789a37cd4 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -318,7 +318,7 @@ class TestArchive(TestIncidents): webhook = MockAsyncWebhook() self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) # Patch in our webhook - # Define our own `incident` for archivation + # Define our own `incident` to be archived incident = MockMessage( content="this is an incident", author=MockUser(name="author_name", avatar_url="author_avatar"), -- cgit v1.2.3 From df1730ef5d51223fe1d5a2cfe8c027e5177ae9c7 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 12 Jul 2020 16:30:03 +0200 Subject: Fix DuckPond tests now that send_webhook is gone. Some of the tests were failing because they were expecting send_webhook to be a method of the DuckPond cog, other tests simply were no longer applicable, and have been removed. https://github.com/python-discord/bot/issues/667 --- tests/bot/cogs/test_duck_pond.py | 51 ++++++++++------------------------------ 1 file changed, 12 insertions(+), 39 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_duck_pond.py b/tests/bot/cogs/test_duck_pond.py index a8c0107c6..cfe10aebf 100644 --- a/tests/bot/cogs/test_duck_pond.py +++ b/tests/bot/cogs/test_duck_pond.py @@ -129,38 +129,6 @@ class DuckPondTests(base.LoggingTestsMixin, unittest.IsolatedAsyncioTestCase): ): self.assertEqual(expected_return, actual_return) - def test_send_webhook_correctly_passes_on_arguments(self): - """The `send_webhook` method should pass the arguments to the webhook correctly.""" - self.cog.webhook = helpers.MockAsyncWebhook() - - content = "fake content" - username = "fake username" - avatar_url = "fake avatar_url" - embed = "fake embed" - - asyncio.run(self.cog.send_webhook(content, username, avatar_url, embed)) - - self.cog.webhook.send.assert_called_once_with( - content=content, - username=username, - avatar_url=avatar_url, - embed=embed - ) - - def test_send_webhook_logs_when_sending_message_fails(self): - """The `send_webhook` method should catch a `discord.HTTPException` and log accordingly.""" - self.cog.webhook = helpers.MockAsyncWebhook() - self.cog.webhook.send.side_effect = discord.HTTPException(response=MagicMock(), message="Something failed.") - - log = logging.getLogger('bot.cogs.duck_pond') - with self.assertLogs(logger=log, level=logging.ERROR) as log_watcher: - asyncio.run(self.cog.send_webhook()) - - self.assertEqual(len(log_watcher.records), 1) - - record = log_watcher.records[0] - self.assertEqual(record.levelno, logging.ERROR) - def _get_reaction( self, emoji: typing.Union[str, helpers.MockEmoji], @@ -280,16 +248,20 @@ class DuckPondTests(base.LoggingTestsMixin, unittest.IsolatedAsyncioTestCase): async def test_relay_message_correctly_relays_content_and_attachments(self): """The `relay_message` method should correctly relay message content and attachments.""" - send_webhook_path = f"{MODULE_PATH}.DuckPond.send_webhook" + send_webhook_path = f"{MODULE_PATH}.send_webhook" send_attachments_path = f"{MODULE_PATH}.send_attachments" + author = MagicMock( + display_name="x", + avatar_url="https://" + ) self.cog.webhook = helpers.MockAsyncWebhook() test_values = ( - (helpers.MockMessage(clean_content="", attachments=[]), False, False), - (helpers.MockMessage(clean_content="message", attachments=[]), True, False), - (helpers.MockMessage(clean_content="", attachments=["attachment"]), False, True), - (helpers.MockMessage(clean_content="message", attachments=["attachment"]), True, True), + (helpers.MockMessage(author=author, clean_content="", attachments=[]), False, False), + (helpers.MockMessage(author=author, clean_content="message", attachments=[]), True, False), + (helpers.MockMessage(author=author, clean_content="", attachments=["attachment"]), False, True), + (helpers.MockMessage(author=author, clean_content="message", attachments=["attachment"]), True, True), ) for message, expect_webhook_call, expect_attachment_call in test_values: @@ -314,14 +286,14 @@ class DuckPondTests(base.LoggingTestsMixin, unittest.IsolatedAsyncioTestCase): for side_effect in side_effects: # pragma: no cover send_attachments.side_effect = side_effect - with patch(f"{MODULE_PATH}.DuckPond.send_webhook", new_callable=AsyncMock) as send_webhook: + with patch(f"{MODULE_PATH}.send_webhook", new_callable=AsyncMock) as send_webhook: with self.subTest(side_effect=type(side_effect).__name__): with self.assertNotLogs(logger=log, level=logging.ERROR): await self.cog.relay_message(message) self.assertEqual(send_webhook.call_count, 2) - @patch(f"{MODULE_PATH}.DuckPond.send_webhook", new_callable=AsyncMock) + @patch(f"{MODULE_PATH}.send_webhook", new_callable=AsyncMock) @patch(f"{MODULE_PATH}.send_attachments", new_callable=AsyncMock) async def test_relay_message_handles_attachment_http_error(self, send_attachments, send_webhook): """The `relay_message` method should handle irretrievable attachments.""" @@ -337,6 +309,7 @@ class DuckPondTests(base.LoggingTestsMixin, unittest.IsolatedAsyncioTestCase): await self.cog.relay_message(message) send_webhook.assert_called_once_with( + webhook=self.cog.webhook, content=message.clean_content, username=message.author.display_name, avatar_url=message.author.avatar_url -- cgit v1.2.3 From c4e9060a76a901c7d2e6035e6ca19d51770a4ab3 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Mon, 13 Jul 2020 15:04:40 +0200 Subject: Incidents: add `download_file` helper & tests Co-authored-by: MarkKoz --- bot/cogs/moderation/incidents.py | 13 +++++++++++++ tests/bot/cogs/moderation/test_incidents.py | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index be46c8202..65b0e458e 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -42,6 +42,19 @@ ALLOWED_ROLES: t.Set[int] = set(Guild.moderation_roles) ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal} +async def download_file(attachment: discord.Attachment) -> t.Optional[discord.File]: + """ + Download & return `attachment` file. + + If the download fails, the reason is logged and None will be returned. + """ + log.debug(f"Attempting to download attachment: {attachment.filename}") + try: + return await attachment.to_file() + except Exception: + log.exception("Failed to download attachment") + + def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> discord.Embed: """ Create an embed representation of `incident` for the #incidents-archive channel. diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 789a37cd4..273916199 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -12,6 +12,7 @@ from bot.cogs.moderation import Incidents, incidents from bot.constants import Colours from tests.helpers import ( MockAsyncWebhook, + MockAttachment, MockBot, MockMember, MockMessage, @@ -69,6 +70,25 @@ mock_404 = discord.NotFound( ) +class TestDownloadFile(unittest.IsolatedAsyncioTestCase): + """Collection of tests for the `download_file` helper function.""" + + async def test_download_file_success(self): + """If `to_file` succeeds, function returns the acquired `discord.File`.""" + file = MagicMock(discord.File, filename="bigbadlemon.jpg") + attachment = MockAttachment(to_file=AsyncMock(return_value=file)) + + acquired_file = await incidents.download_file(attachment) + self.assertIs(file, acquired_file) + + async def test_download_file_fail(self): + """If `to_file` fails, function handles the exception & returns None.""" + attachment = MockAttachment(to_file=AsyncMock(side_effect=mock_404)) + + acquired_file = await incidents.download_file(attachment) + self.assertIsNone(acquired_file) + + class TestMakeEmbed(unittest.TestCase): """Collection of tests for the `make_embed` helper function.""" -- cgit v1.2.3 From f1b1d0cb723abbbf7d4b49ac4b42fe0b7f266692 Mon Sep 17 00:00:00 2001 From: Slushie Date: Mon, 13 Jul 2020 16:09:08 +0100 Subject: edit snekbox tests to work with filtering --- tests/bot/cogs/test_snekbox.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_snekbox.py b/tests/bot/cogs/test_snekbox.py index cf9adbee0..98dee7a1b 100644 --- a/tests/bot/cogs/test_snekbox.py +++ b/tests/bot/cogs/test_snekbox.py @@ -233,6 +233,10 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): self.cog.get_status_emoji = MagicMock(return_value=':yay!:') self.cog.format_output = AsyncMock(return_value=('[No output]', None)) + mocked_filter_cog = MagicMock() + mocked_filter_cog.filter_eval = AsyncMock(return_value=False) + self.bot.get_cog.return_value = mocked_filter_cog + await self.cog.send_eval(ctx, 'MyAwesomeCode') ctx.send.assert_called_once_with( '@LemonLemonishBeard#0042 :yay!: Return code 0.\n\n```py\n[No output]\n```' @@ -254,6 +258,10 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): self.cog.get_status_emoji = MagicMock(return_value=':yay!:') self.cog.format_output = AsyncMock(return_value=('Way too long beard', 'lookatmybeard.com')) + mocked_filter_cog = MagicMock() + mocked_filter_cog.filter_eval = AsyncMock(return_value=False) + self.bot.get_cog.return_value = mocked_filter_cog + await self.cog.send_eval(ctx, 'MyAwesomeCode') ctx.send.assert_called_once_with( '@LemonLemonishBeard#0042 :yay!: Return code 0.' @@ -275,6 +283,10 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): self.cog.get_status_emoji = MagicMock(return_value=':nope!:') self.cog.format_output = AsyncMock() # This function isn't called + mocked_filter_cog = MagicMock() + mocked_filter_cog.filter_eval = AsyncMock(return_value=False) + self.bot.get_cog.return_value = mocked_filter_cog + await self.cog.send_eval(ctx, 'MyAwesomeCode') ctx.send.assert_called_once_with( '@LemonLemonishBeard#0042 :nope!: Return code 127.\n\n```py\nBeard got stuck in the eval\n```' -- cgit v1.2.3 From 6e48d666b31d13d801c394b527ce545b039b478f Mon Sep 17 00:00:00 2001 From: kwzrd Date: Tue, 14 Jul 2020 17:28:53 +0200 Subject: Incidents: link `proxy_url` if attachment fails to download Suggested by Mark during review. If the download fails, we fallback on showing an informative message, which will link the attachment cdn link. The attachment-handling logic was moved from the `archive` coroutine into `make_embed`, which now also returns the file, if available. In the end, this appears to be the smoothest approach. Co-authored-by: MarkKoz --- bot/cogs/moderation/incidents.py | 36 +++++++++----- tests/bot/cogs/moderation/test_incidents.py | 73 ++++++++++++++--------------- 2 files changed, 59 insertions(+), 50 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 65b0e458e..018538040 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -41,6 +41,10 @@ ALLOWED_ROLES: t.Set[int] = set(Guild.moderation_roles) # Message must have all of these emoji to pass the `has_signals` check ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal} +# An embed coupled with an optional file to be dispatched +# If the file is not None, the embed attempts to show it in its body +FileEmbed = t.Tuple[discord.Embed, t.Optional[discord.File]] + async def download_file(attachment: discord.Attachment) -> t.Optional[discord.File]: """ @@ -55,7 +59,7 @@ async def download_file(attachment: discord.Attachment) -> t.Optional[discord.Fi log.exception("Failed to download attachment") -def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> discord.Embed: +async def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> FileEmbed: """ Create an embed representation of `incident` for the #incidents-archive channel. @@ -66,6 +70,11 @@ def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord. of information will be relayed in other ways, e.g. webhook username. As mentions in embeds do not ping, we do not need to use `incident.clean_content`. + + If `incident` contains attachments, the first attachment will be downloaded and + returned alongside the embed. The embed attempts to display the attachment. + Should the download fail, we fallback on linking the `proxy_url`, which should + remain functional for some time after the original message is deleted. """ log.trace(f"Creating embed for {incident.id=}") @@ -83,7 +92,18 @@ def make_embed(incident: discord.Message, outcome: Signal, actioned_by: discord. ) embed.set_footer(text=footer, icon_url=actioned_by.avatar_url) - return embed + if incident.attachments: + attachment = incident.attachments[0] # User-sent messages can only contain one attachment + file = await download_file(attachment) + + if file is not None: + embed.set_image(url=f"attachment://{attachment.filename}") # Embed displays the attached file + else: + embed.set_author(name="[Failed to relay attachment]", url=attachment.proxy_url) # Embed links the file + else: + file = None + + return embed, file def is_incident(message: discord.Message) -> bool: @@ -215,17 +235,7 @@ class Incidents(Cog): message is not safe to be deleted, as we will lose some information. """ log.debug(f"Archiving incident: {incident.id} (outcome: {outcome}, actioned by: {actioned_by})") - embed = make_embed(incident, outcome, actioned_by) - - # If the incident had an attachment, we will try to relay it - if incident.attachments: - attachment = incident.attachments[0] # User-sent messages can only contain one attachment - log.debug(f"Attempting to archive incident attachment: {attachment.filename}") - - attachment_file = await attachment.to_file() # The file will be sent with the webhook - embed.set_image(url=f"attachment://{attachment.filename}") # Embed displays the attached file - else: - attachment_file = None + embed, attachment_file = await make_embed(incident, outcome, actioned_by) try: webhook = await self.bot.fetch_webhook(Webhooks.incidents_archive) diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 273916199..9b6054f55 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -89,30 +89,58 @@ class TestDownloadFile(unittest.IsolatedAsyncioTestCase): self.assertIsNone(acquired_file) -class TestMakeEmbed(unittest.TestCase): +class TestMakeEmbed(unittest.IsolatedAsyncioTestCase): """Collection of tests for the `make_embed` helper function.""" - def test_make_embed_actioned(self): + async def test_make_embed_actioned(self): """Embed is coloured green and footer contains 'Actioned' when `outcome=Signal.ACTIONED`.""" - embed = incidents.make_embed(MockMessage(), incidents.Signal.ACTIONED, MockMember()) + embed, file = await incidents.make_embed(MockMessage(), incidents.Signal.ACTIONED, MockMember()) self.assertEqual(embed.colour.value, Colours.soft_green) self.assertIn("Actioned", embed.footer.text) - def test_make_embed_not_actioned(self): + async def test_make_embed_not_actioned(self): """Embed is coloured red and footer contains 'Rejected' when `outcome=Signal.NOT_ACTIONED`.""" - embed = incidents.make_embed(MockMessage(), incidents.Signal.NOT_ACTIONED, MockMember()) + embed, file = await incidents.make_embed(MockMessage(), incidents.Signal.NOT_ACTIONED, MockMember()) self.assertEqual(embed.colour.value, Colours.soft_red) self.assertIn("Rejected", embed.footer.text) - def test_make_embed_content(self): + async def test_make_embed_content(self): """Incident content appears as embed description.""" incident = MockMessage(content="this is an incident") - embed = incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember()) + embed, file = await incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember()) self.assertEqual(incident.content, embed.description) + async def test_make_embed_with_attachment_succeeds(self): + """Incident's attachment is downloaded and displayed in the embed's image field.""" + file = MagicMock(discord.File, filename="bigbadjoe.jpg") + attachment = MockAttachment(filename="bigbadjoe.jpg") + incident = MockMessage(content="this is an incident", attachments=[attachment]) + + # Patch `download_file` to return our `file` + with patch("bot.cogs.moderation.incidents.download_file", AsyncMock(return_value=file)): + embed, returned_file = await incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember()) + + self.assertIs(file, returned_file) + self.assertEqual("attachment://bigbadjoe.jpg", embed.image.url) + + async def test_make_embed_with_attachment_fails(self): + """Incident's attachment fails to download, proxy url is linked instead.""" + attachment = MockAttachment(proxy_url="discord.com/bigbadjoe.jpg") + incident = MockMessage(content="this is an incident", attachments=[attachment]) + + # Patch `download_file` to return None as if the download failed + with patch("bot.cogs.moderation.incidents.download_file", AsyncMock(return_value=None)): + embed, returned_file = await incidents.make_embed(incident, incidents.Signal.ACTIONED, MockMember()) + + self.assertIsNone(returned_file) + + # The author name field is simply expected to have something in it, we do not assert the message + self.assertGreater(len(embed.author.name), 0) + self.assertEqual(embed.author.url, "discord.com/bigbadjoe.jpg") # However, it should link the exact url + @patch("bot.constants.Channels.incidents", 123) class TestIsIncident(unittest.TestCase): @@ -343,11 +371,10 @@ class TestArchive(TestIncidents): content="this is an incident", author=MockUser(name="author_name", avatar_url="author_avatar"), id=123, - attachments=[], # This incident has no attachments ) built_embed = MagicMock(discord.Embed, id=123) # We patch `make_embed` to return this - with patch("bot.cogs.moderation.incidents.make_embed", MagicMock(return_value=built_embed)): + with patch("bot.cogs.moderation.incidents.make_embed", AsyncMock(return_value=(built_embed, None))): archive_return = await self.cog_instance.archive(incident, MagicMock(value="A"), MockMember()) # Now we check that the webhook was given the correct args, and that `archive` returned True @@ -359,34 +386,6 @@ class TestArchive(TestIncidents): ) self.assertTrue(archive_return) - async def test_archive_relays_incident_with_attachments(self): - """ - Incident attachments are relayed and displayed in the embed. - - This test asserts the two things that need to happen in order to relay the attachment. - The embed returned by `make_embed` must have the `set_image` method called with the - attachment's filename, and the file must be passed to the webhook's send method. - """ - attachment_file = MagicMock(discord.File) - attachment = MagicMock( - discord.Attachment, - filename="abc.png", - to_file=AsyncMock(return_value=attachment_file), - ) - incident = MockMessage( - attachments=[attachment], - ) - built_embed = MagicMock(discord.Embed) - - with patch("bot.cogs.moderation.incidents.make_embed", MagicMock(return_value=built_embed)): - await self.cog_instance.archive(incident, incidents.Signal.ACTIONED, actioned_by=MockMember()) - - built_embed.set_image.assert_called_once_with(url="attachment://abc.png") - - send_kwargs = self.cog_instance.bot.fetch_webhook.return_value.send.call_args.kwargs - self.assertIn("file", send_kwargs) - self.assertIs(send_kwargs["file"], attachment_file) - async def test_archive_clyde_username(self): """ The archive webhook username is cleansed using `sub_clyde`. -- cgit v1.2.3 From 6f5fb205bcc3f9b468ef585f83e123e5b19d7340 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Thu, 16 Jul 2020 17:03:02 +0200 Subject: Incidents: reduce log level of 404 exception Co-authored-by: MarkKoz --- bot/cogs/moderation/incidents.py | 2 ++ tests/bot/cogs/moderation/test_incidents.py | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 018538040..2d5f26f20 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -55,6 +55,8 @@ async def download_file(attachment: discord.Attachment) -> t.Optional[discord.Fi log.debug(f"Attempting to download attachment: {attachment.filename}") try: return await attachment.to_file() + except discord.NotFound as not_found: + log.debug(f"Failed to download attachment: {not_found}") except Exception: log.exception("Failed to download attachment") diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 9b6054f55..435a1cd51 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -81,13 +81,23 @@ class TestDownloadFile(unittest.IsolatedAsyncioTestCase): acquired_file = await incidents.download_file(attachment) self.assertIs(file, acquired_file) - async def test_download_file_fail(self): - """If `to_file` fails, function handles the exception & returns None.""" + async def test_download_file_404(self): + """If `to_file` encounters a 404, function handles the exception & returns None.""" attachment = MockAttachment(to_file=AsyncMock(side_effect=mock_404)) acquired_file = await incidents.download_file(attachment) self.assertIsNone(acquired_file) + async def test_download_file_fail(self): + """If `to_file` fails on a non-404 error, function logs the exception & returns None.""" + arbitrary_error = discord.HTTPException(MagicMock(aiohttp.ClientResponse), "Arbitrary API error") + attachment = MockAttachment(to_file=AsyncMock(side_effect=arbitrary_error)) + + with self.assertLogs(logger=incidents.log, level=logging.ERROR): + acquired_file = await incidents.download_file(attachment) + + self.assertIsNone(acquired_file) + class TestMakeEmbed(unittest.IsolatedAsyncioTestCase): """Collection of tests for the `make_embed` helper function.""" -- cgit v1.2.3 From 1c569f2f38fe18d6210deec001046cf9ee68ea53 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sat, 18 Jul 2020 16:54:01 +0200 Subject: Remove AntiMalWare constants, use cache data. Also updates the tests for this cog. --- bot/bot.py | 2 +- bot/cogs/antimalware.py | 24 ++++++++++++++---------- bot/constants.py | 6 ------ config-default.yml | 29 ----------------------------- tests/bot/cogs/test_antimalware.py | 24 +++++++++++++++--------- 5 files changed, 30 insertions(+), 55 deletions(-) (limited to 'tests') diff --git a/bot/bot.py b/bot/bot.py index 6c02e72a7..962c8dd93 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -34,6 +34,7 @@ class Bot(commands.Bot): self.redis_ready = asyncio.Event() self.redis_closed = False self.api_client = api.APIClient(loop=self.loop) + self.allow_deny_list_cache = {} self._connector = None self._resolver = None @@ -52,7 +53,6 @@ class Bot(commands.Bot): async def _cache_allow_deny_list_data(self) -> None: """Cache all the data in the AllowDenyList on the site.""" full_cache = await self.api_client.get('bot/allow_deny_lists') - self.allow_deny_list_cache = {} for item in full_cache: type_ = item.get("type") diff --git a/bot/cogs/antimalware.py b/bot/cogs/antimalware.py index ea257442e..38ff1133d 100644 --- a/bot/cogs/antimalware.py +++ b/bot/cogs/antimalware.py @@ -6,7 +6,7 @@ from discord import Embed, Message, NotFound from discord.ext.commands import Cog from bot.bot import Bot -from bot.constants import AntiMalware as AntiMalwareConfig, Channels, STAFF_ROLES, URLs +from bot.constants import Channels, STAFF_ROLES, URLs log = logging.getLogger(__name__) @@ -27,7 +27,7 @@ TXT_EMBED_DESCRIPTION = ( 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" + "We currently allow the following file types: **{joined_whitelist}**.\n\n" "Feel free to ask in {meta_channel_mention} if you think this is a mistake." ) @@ -38,6 +38,16 @@ class AntiMalware(Cog): def __init__(self, bot: Bot): self.bot = bot + def _get_whitelisted_file_formats(self) -> list: + """Get the file formats currently on the whitelist.""" + return [item.get('content') for item in self.bot.allow_deny_list_cache['file_format.True']] + + def _get_disallowed_extensions(self, message: Message) -> t.Iterable[str]: + """Get an iterable containing all the disallowed extensions of attachments.""" + file_extensions = {splitext(attachment.filename.lower())[1] for attachment in message.attachments} + extensions_blocked = file_extensions - set(self._get_whitelisted_file_formats()) + return extensions_blocked + @Cog.listener() async def on_message(self, message: Message) -> None: """Identify messages with prohibited attachments.""" @@ -51,7 +61,7 @@ class AntiMalware(Cog): return embed = Embed() - extensions_blocked = self.get_disallowed_extensions(message) + extensions_blocked = self._get_disallowed_extensions(message) blocked_extensions_str = ', '.join(extensions_blocked) if ".py" in extensions_blocked: # Short-circuit on *.py files to provide a pastebin link @@ -63,6 +73,7 @@ class AntiMalware(Cog): elif extensions_blocked: meta_channel = self.bot.get_channel(Channels.meta) embed.description = DISALLOWED_EMBED_DESCRIPTION.format( + joined_whitelist=', '.join(self._get_whitelisted_file_formats()), blocked_extensions_str=blocked_extensions_str, meta_channel_mention=meta_channel.mention, ) @@ -81,13 +92,6 @@ class AntiMalware(Cog): except NotFound: log.info(f"Tried to delete message `{message.id}`, but message could not be found.") - @classmethod - def get_disallowed_extensions(cls, message: Message) -> t.Iterable[str]: - """Get an iterable containing all the disallowed extensions of attachments.""" - file_extensions = {splitext(attachment.filename.lower())[1] for attachment in message.attachments} - extensions_blocked = file_extensions - set(AntiMalwareConfig.whitelist) - return extensions_blocked - def setup(bot: Bot) -> None: """Load the AntiMalware cog.""" diff --git a/bot/constants.py b/bot/constants.py index f5245ca50..857e6c4f0 100644 --- a/bot/constants.py +++ b/bot/constants.py @@ -527,12 +527,6 @@ class AntiSpam(metaclass=YAMLGetter): rules: Dict[str, Dict[str, int]] -class AntiMalware(metaclass=YAMLGetter): - section = "anti_malware" - - whitelist: list - - class BigBrother(metaclass=YAMLGetter): section = 'big_brother' diff --git a/config-default.yml b/config-default.yml index 81c8c40d5..503cc2b52 100644 --- a/config-default.yml +++ b/config-default.yml @@ -386,35 +386,6 @@ anti_spam: max: 3 -anti_malware: - whitelist: - - '.3gp' - - '.3g2' - - '.avi' - - '.bmp' - - '.gif' - - '.h264' - - '.jpg' - - '.jpeg' - - '.m4v' - - '.mkv' - - '.mov' - - '.mp4' - - '.mpeg' - - '.mpg' - - '.png' - - '.tiff' - - '.wmv' - - '.svg' - - '.psd' # Photoshop - - '.ai' # Illustrator - - '.aep' # After Effects - - '.xcf' # GIMP - - '.mp3' - - '.wav' - - '.ogg' - - reddit: subreddits: - 'r/Python' diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index f219fc1ba..1e010d2ce 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -1,28 +1,33 @@ import unittest -from unittest.mock import AsyncMock, Mock, patch +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 +from bot.constants import Channels, STAFF_ROLES 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.""" def setUp(self): """Sets up fresh objects for each test.""" self.bot = MockBot() + self.bot.allow_deny_list_cache = { + "file_format.True": [ + {"content": ".first"}, + {"content": ".second"}, + {"content": ".third"} + ] + } self.cog = antimalware.AntiMalware(self.bot) self.message = MockMessage() + self.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="python.first") self.message.attachments = [attachment] await self.cog.on_message(self.message) @@ -93,7 +98,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): 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): + async def test_other_disallowed_extension_embed_description(self): """Test the description for a non .py/.txt disallowed extension.""" attachment = MockAttachment(filename="python.disallowed") self.message.attachments = [attachment] @@ -109,6 +114,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(embed.description, antimalware.DISALLOWED_EMBED_DESCRIPTION.format.return_value) antimalware.DISALLOWED_EMBED_DESCRIPTION.format.assert_called_with( + joined_whitelist=", ".join(self.whitelist), blocked_extensions_str=".disallowed", meta_channel_mention=meta_channel.mention ) @@ -135,7 +141,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): """The return value should include all non-whitelisted extensions.""" test_values = ( ([], []), - (AntiMalwareConfig.whitelist, []), + (self.whitelist, []), ([".first"], []), ([".first", ".disallowed"], [".disallowed"]), ([".disallowed"], [".disallowed"]), @@ -145,7 +151,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): 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) + disallowed_extensions = self.cog._get_disallowed_extensions(self.message) self.assertCountEqual(disallowed_extensions, expected_disallowed_extensions) -- cgit v1.2.3 From e93fdaf57d1d35394b466a6bd1c84712e29415d7 Mon Sep 17 00:00:00 2001 From: wookie184 Date: Mon, 20 Jul 2020 16:05:32 +0100 Subject: Edited tests to reflect changes (removed py formatting) --- tests/bot/cogs/test_snekbox.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_snekbox.py b/tests/bot/cogs/test_snekbox.py index 98dee7a1b..343e37db9 100644 --- a/tests/bot/cogs/test_snekbox.py +++ b/tests/bot/cogs/test_snekbox.py @@ -239,7 +239,7 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): await self.cog.send_eval(ctx, 'MyAwesomeCode') ctx.send.assert_called_once_with( - '@LemonLemonishBeard#0042 :yay!: Return code 0.\n\n```py\n[No output]\n```' + '@LemonLemonishBeard#0042 :yay!: Return code 0.\n\n```\n[No output]\n```' ) self.cog.post_eval.assert_called_once_with('MyAwesomeCode') self.cog.get_status_emoji.assert_called_once_with({'stdout': '', 'returncode': 0}) @@ -265,7 +265,7 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): await self.cog.send_eval(ctx, 'MyAwesomeCode') ctx.send.assert_called_once_with( '@LemonLemonishBeard#0042 :yay!: Return code 0.' - '\n\n```py\nWay too long beard\n```\nFull output: lookatmybeard.com' + '\n\n```\nWay too long beard\n```\nFull output: lookatmybeard.com' ) self.cog.post_eval.assert_called_once_with('MyAwesomeCode') self.cog.get_status_emoji.assert_called_once_with({'stdout': 'Way too long beard', 'returncode': 0}) @@ -289,7 +289,7 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase): await self.cog.send_eval(ctx, 'MyAwesomeCode') ctx.send.assert_called_once_with( - '@LemonLemonishBeard#0042 :nope!: Return code 127.\n\n```py\nBeard got stuck in the eval\n```' + '@LemonLemonishBeard#0042 :nope!: Return code 127.\n\n```\nBeard got stuck in the eval\n```' ) self.cog.post_eval.assert_called_once_with('MyAwesomeCode') self.cog.get_status_emoji.assert_called_once_with({'stdout': 'ERROR', 'returncode': 127}) -- cgit v1.2.3 From 63c7827d9d9025c7505747904237b37eb46464df Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 22 Jul 2020 11:42:31 -0700 Subject: Jam Tests: fix utils patch stop needs to be called on the patcher, not the mock. Furthermore, using addCleanup is safer than tearDown because the latter may not be called if an exception is raised in setUp. --- tests/bot/cogs/test_jams.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 2f2cb4695..28eb1ab53 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -16,11 +16,12 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.guild = MockGuild([self.admin_role]) self.ctx = MockContext(bot=self.bot, author=self.command_user, guild=self.guild) self.cog = CodeJams(self.bot) - self.utils_mock = patch("bot.cogs.jams.utils").start() - self.default_args = [self.cog, self.ctx, "foo"] - def tearDown(self): - self.utils_mock.stop() + utils_patcher = patch("bot.cogs.jams.utils") + self.utils_mock = utils_patcher.start() + self.addCleanup(utils_patcher.stop) + + self.default_args = [self.cog, self.ctx, "foo"] async def test_too_small_amount_of_team_members_passed(self): """Should `ctx.send` and exit early when too small amount of members.""" -- cgit v1.2.3 From f7e177357e7a47d9a43b492aac7703961af72c19 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 22 Jul 2020 11:43:58 -0700 Subject: Jam Tests: re-arrange tests to follow definition order in the cog --- tests/bot/cogs/test_jams.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 28eb1ab53..e0018e006 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -48,6 +48,16 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.cog.create_channels.assert_not_awaited() self.cog.add_roles.assert_not_awaited() + async def test_result_sending(self): + """Should call `ctx.send` when everything goes right.""" + self.cog.create_channels = AsyncMock() + self.cog.add_roles = AsyncMock() + members = [MockMember() for _ in range(5)] + await self.cog.createteam(self.cog, self.ctx, "foo", members) + self.cog.create_channels.assert_awaited_once() + self.cog.add_roles.assert_awaited_once() + self.ctx.send.assert_awaited_once() + async def test_category_dont_exist(self): """Should create code jam category.""" self.utils_mock.get.return_value = None @@ -125,16 +135,6 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): for member in members: member.add_roles.assert_any_await(jam_role) - async def test_result_sending(self): - """Should call `ctx.send` when everything goes right.""" - self.cog.create_channels = AsyncMock() - self.cog.add_roles = AsyncMock() - members = [MockMember() for _ in range(5)] - await self.cog.createteam(self.cog, self.ctx, "foo", members) - self.cog.create_channels.assert_awaited_once() - self.cog.add_roles.assert_awaited_once() - self.ctx.send.assert_awaited_once() - class CodeJamSetup(unittest.TestCase): """Test for `setup` function of `CodeJam` cog.""" -- cgit v1.2.3 From b1d0f36356ecf4eee729bf276c8b0ed10653ad54 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 22 Jul 2020 11:47:49 -0700 Subject: Jam Tests: remove default_args attribute Kind of redundant since it's only used by two tests. --- tests/bot/cogs/test_jams.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index e0018e006..0fce2a67c 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -21,8 +21,6 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.utils_mock = utils_patcher.start() self.addCleanup(utils_patcher.stop) - self.default_args = [self.cog, self.ctx, "foo"] - async def test_too_small_amount_of_team_members_passed(self): """Should `ctx.send` and exit early when too small amount of members.""" for case in (1, 2): @@ -32,7 +30,8 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.ctx.reset_mock() self.utils_mock.reset_mock() - await self.cog.createteam(*self.default_args, (MockMember() for _ in range(case))) + members = (MockMember() for _ in range(case)) + await self.cog.createteam(self.cog, self.ctx, "foo", members) self.ctx.send.assert_awaited_once() self.cog.create_channels.assert_not_awaited() @@ -43,7 +42,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.cog.create_channels = AsyncMock() self.cog.add_roles = AsyncMock() member = MockMember() - await self.cog.createteam(*self.default_args, (member for _ in range(5))) + await self.cog.createteam(self.cog, self.ctx, "foo", (member for _ in range(5))) self.ctx.send.assert_awaited_once() self.cog.create_channels.assert_not_awaited() self.cog.add_roles.assert_not_awaited() -- cgit v1.2.3 From 44cd1d989d491d692d48324228ccc9593a545cd2 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 22 Jul 2020 11:51:08 -0700 Subject: Jam Tests: space out lines for readability --- tests/bot/cogs/test_jams.py | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 0fce2a67c..81fbcb798 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -41,8 +41,10 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should `ctx.send` and exit early because duplicate members provided and total there is only 1 member.""" self.cog.create_channels = AsyncMock() self.cog.add_roles = AsyncMock() + member = MockMember() await self.cog.createteam(self.cog, self.ctx, "foo", (member for _ in range(5))) + self.ctx.send.assert_awaited_once() self.cog.create_channels.assert_not_awaited() self.cog.add_roles.assert_not_awaited() @@ -51,8 +53,10 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Should call `ctx.send` when everything goes right.""" self.cog.create_channels = AsyncMock() self.cog.add_roles = AsyncMock() + members = [MockMember() for _ in range(5)] await self.cog.createteam(self.cog, self.ctx, "foo", members) + self.cog.create_channels.assert_awaited_once() self.cog.add_roles.assert_awaited_once() self.ctx.send.assert_awaited_once() @@ -60,7 +64,9 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_category_dont_exist(self): """Should create code jam category.""" self.utils_mock.get.return_value = None + await self.cog.get_category(self.guild) + self.guild.create_category_channel.assert_awaited_once() category_overwrites = self.guild.create_category_channel.call_args[1]["overwrites"] -- cgit v1.2.3 From 12168766a153d9d1bd134ff64f74997eef8ff7b0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 22 Jul 2020 16:11:32 -0700 Subject: Jam tests: fix category test --- tests/bot/cogs/test_jams.py | 38 ++++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 14 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 81fbcb798..54a096703 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -1,11 +1,22 @@ import unittest -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, create_autospec -from bot.cogs.jams import CodeJams, setup +from discord import CategoryChannel + +from bot.cogs import jams from bot.constants import Roles from tests.helpers import MockBot, MockContext, MockGuild, MockMember, MockRole, MockTextChannel +def get_mock_category(channel_count: int, name: str) -> CategoryChannel: + """Return a mocked code jam category.""" + category = create_autospec(CategoryChannel, spec_set=True, instance=True) + category.name = name + category.channels = [MockTextChannel() for _ in range(channel_count)] + + return category + + class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): """Tests for `createteam` command.""" @@ -15,11 +26,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.command_user = MockMember([self.admin_role]) self.guild = MockGuild([self.admin_role]) self.ctx = MockContext(bot=self.bot, author=self.command_user, guild=self.guild) - self.cog = CodeJams(self.bot) - - utils_patcher = patch("bot.cogs.jams.utils") - self.utils_mock = utils_patcher.start() - self.addCleanup(utils_patcher.stop) + self.cog = jams.CodeJams(self.bot) async def test_too_small_amount_of_team_members_passed(self): """Should `ctx.send` and exit early when too small amount of members.""" @@ -29,7 +36,6 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.cog.add_roles = AsyncMock() self.ctx.reset_mock() - self.utils_mock.reset_mock() members = (MockMember() for _ in range(case)) await self.cog.createteam(self.cog, self.ctx, "foo", members) @@ -63,8 +69,6 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_category_dont_exist(self): """Should create code jam category.""" - self.utils_mock.get.return_value = None - await self.cog.get_category(self.guild) self.guild.create_category_channel.assert_awaited_once() @@ -75,8 +79,15 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_category_channel_exist(self): """Should not try to create category channel.""" - await self.cog.get_category(self.guild) - self.guild.create_category_channel.assert_not_awaited() + expected_category = get_mock_category(48, jams.CATEGORY_NAME) + self.guild.categories = [ + get_mock_category(48, "other"), + expected_category, + get_mock_category(6, jams.CATEGORY_NAME), + ] + + actual_category = await self.cog.get_category(self.guild) + self.assertEqual(expected_category, actual_category) async def test_channel_overwrites(self): """Should have correct permission overwrites for users and roles.""" @@ -103,7 +114,6 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_team_channels_creation(self): """Should create new voice and text channel for team.""" - self.utils_mock.get.return_value = "foo" members = [MockMember() for _ in range(5)] self.cog.get_overwrites = MagicMock() @@ -147,5 +157,5 @@ class CodeJamSetup(unittest.TestCase): def test_setup(self): """Should call `bot.add_cog`.""" bot = MockBot() - setup(bot) + jams.setup(bot) bot.add_cog.assert_called_once() -- cgit v1.2.3 From 92d3f88eb5c2348f3e4cb53a22a833bed61c6fb7 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 22 Jul 2020 16:21:38 -0700 Subject: Jam tests: add subtests to non-existent category test The test has to account for not only the name not matching, but also a lack of available spaces for new channels. --- tests/bot/cogs/test_jams.py | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index 54a096703..e6b2ac588 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -67,15 +67,26 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.cog.add_roles.assert_awaited_once() self.ctx.send.assert_awaited_once() - async def test_category_dont_exist(self): - """Should create code jam category.""" - await self.cog.get_category(self.guild) + async def test_category_doesnt_exist(self): + """Should create a new code jam category.""" + subtests = ( + [], + [get_mock_category(jams.MAX_CHANNELS - 1, jams.CATEGORY_NAME)], + [get_mock_category(48, "other")], + ) + + for categories in subtests: + self.guild.reset_mock() + self.guild.categories = categories + + with self.subTest(categories=categories): + await self.cog.get_category(self.guild) - self.guild.create_category_channel.assert_awaited_once() - category_overwrites = self.guild.create_category_channel.call_args[1]["overwrites"] + self.guild.create_category_channel.assert_awaited_once() + category_overwrites = self.guild.create_category_channel.call_args[1]["overwrites"] - self.assertFalse(category_overwrites[self.guild.default_role].read_messages) - self.assertTrue(category_overwrites[self.guild.me].read_messages) + self.assertFalse(category_overwrites[self.guild.default_role].read_messages) + self.assertTrue(category_overwrites[self.guild.me].read_messages) async def test_category_channel_exist(self): """Should not try to create category channel.""" -- cgit v1.2.3 From ddba3f5fcfbda0f72baa3f15055c8a92e94c6d88 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 22 Jul 2020 16:27:02 -0700 Subject: Jam tests: assert equality of new category --- tests/bot/cogs/test_jams.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index e6b2ac588..a76a8a051 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -80,13 +80,14 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): self.guild.categories = categories with self.subTest(categories=categories): - await self.cog.get_category(self.guild) + actual_category = await self.cog.get_category(self.guild) self.guild.create_category_channel.assert_awaited_once() category_overwrites = self.guild.create_category_channel.call_args[1]["overwrites"] self.assertFalse(category_overwrites[self.guild.default_role].read_messages) self.assertTrue(category_overwrites[self.guild.me].read_messages) + self.assertEqual(self.guild.create_category_channel.return_value, actual_category) async def test_category_channel_exist(self): """Should not try to create category channel.""" -- cgit v1.2.3 From 8e3c05210f057ab76d135afbe12035847c9029f4 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 22 Jul 2020 16:54:53 -0700 Subject: Jam tests: use the MAX_CHANNELS constant more It's clearer to write MAX_CHANNELS - 2 than a literal 48. --- tests/bot/cogs/test_jams.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_jams.py b/tests/bot/cogs/test_jams.py index a76a8a051..b4ad8535f 100644 --- a/tests/bot/cogs/test_jams.py +++ b/tests/bot/cogs/test_jams.py @@ -72,7 +72,7 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): subtests = ( [], [get_mock_category(jams.MAX_CHANNELS - 1, jams.CATEGORY_NAME)], - [get_mock_category(48, "other")], + [get_mock_category(jams.MAX_CHANNELS - 2, "other")], ) for categories in subtests: @@ -91,11 +91,11 @@ class JamCreateTeamTests(unittest.IsolatedAsyncioTestCase): async def test_category_channel_exist(self): """Should not try to create category channel.""" - expected_category = get_mock_category(48, jams.CATEGORY_NAME) + expected_category = get_mock_category(jams.MAX_CHANNELS - 2, jams.CATEGORY_NAME) self.guild.categories = [ - get_mock_category(48, "other"), + get_mock_category(jams.MAX_CHANNELS - 2, "other"), expected_category, - get_mock_category(6, jams.CATEGORY_NAME), + get_mock_category(0, jams.CATEGORY_NAME), ] actual_category = await self.cog.get_category(self.guild) -- cgit v1.2.3 From 3d5faa421756fadb42590db92e8fee64578390d4 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Mon, 27 Jul 2020 10:26:10 +0200 Subject: Rename AllowDenyList to FilterLists --- bot/__main__.py | 2 +- bot/bot.py | 14 +-- bot/cogs/allow_deny_lists.py | 218 ------------------------------------- bot/cogs/antimalware.py | 2 +- bot/cogs/filter_lists.py | 218 +++++++++++++++++++++++++++++++++++++ bot/cogs/filtering.py | 16 +-- bot/converters.py | 10 +- tests/bot/cogs/test_antimalware.py | 2 +- 8 files changed, 241 insertions(+), 241 deletions(-) delete mode 100644 bot/cogs/allow_deny_lists.py create mode 100644 bot/cogs/filter_lists.py (limited to 'tests') diff --git a/bot/__main__.py b/bot/__main__.py index 932aa705c..c2271cd16 100644 --- a/bot/__main__.py +++ b/bot/__main__.py @@ -53,7 +53,7 @@ bot.load_extension("bot.cogs.verification") # Feature cogs bot.load_extension("bot.cogs.alias") -bot.load_extension("bot.cogs.allow_deny_lists") +bot.load_extension("bot.cogs.filter_lists") bot.load_extension("bot.cogs.defcon") bot.load_extension("bot.cogs.dm_relay") bot.load_extension("bot.cogs.duck_pond") diff --git a/bot/bot.py b/bot/bot.py index d834c151b..3dfb4e948 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -34,7 +34,7 @@ class Bot(commands.Bot): self.redis_ready = asyncio.Event() self.redis_closed = False self.api_client = api.APIClient(loop=self.loop) - self.allow_deny_list_cache = {} + self.filter_list_cache = {} self._connector = None self._resolver = None @@ -50,9 +50,9 @@ class Bot(commands.Bot): self.stats = AsyncStatsClient(self.loop, statsd_url, 8125, prefix="bot") - async def _cache_allow_deny_list_data(self) -> None: - """Cache all the data in the AllowDenyList on the site.""" - full_cache = await self.api_client.get('bot/allow_deny_lists') + async def _cache_filter_list_data(self) -> None: + """Cache all the data in the FilterList on the site.""" + full_cache = await self.api_client.get('bot/filter-lists') for item in full_cache: type_ = item.get("type") @@ -64,7 +64,7 @@ class Bot(commands.Bot): "created_at": item.get("created_at"), "updated_at": item.get("updated_at"), } - self.allow_deny_list_cache.setdefault(f"{type_}.{allowed}", []).append(metadata) + self.filter_list_cache.setdefault(f"{type_}.{allowed}", []).append(metadata) async def _create_redis_session(self) -> None: """ @@ -176,8 +176,8 @@ class Bot(commands.Bot): self.http_session = aiohttp.ClientSession(connector=self._connector) self.api_client.recreate(force=True, connector=self._connector) - # Build the AllowDenyList cache - self.loop.create_task(self._cache_allow_deny_list_data()) + # Build the FilterList cache + self.loop.create_task(self._cache_filter_list_data()) async def on_guild_available(self, guild: discord.Guild) -> None: """ diff --git a/bot/cogs/allow_deny_lists.py b/bot/cogs/allow_deny_lists.py deleted file mode 100644 index e28e32bd6..000000000 --- a/bot/cogs/allow_deny_lists.py +++ /dev/null @@ -1,218 +0,0 @@ -import logging -from typing import Optional - -from discord import Colour, Embed -from discord.ext.commands import BadArgument, Cog, Context, IDConverter, group - -from bot import constants -from bot.api import ResponseCodeError -from bot.bot import Bot -from bot.converters import ValidAllowDenyListType, ValidDiscordServerInvite -from bot.pagination import LinePaginator -from bot.utils.checks import with_role_check - -log = logging.getLogger(__name__) - - -class AllowDenyLists(Cog): - """Commands for blacklisting and whitelisting things.""" - - def __init__(self, bot: Bot) -> None: - self.bot = bot - - async def _add_data( - self, - ctx: Context, - allowed: bool, - list_type: ValidAllowDenyListType, - content: str, - comment: Optional[str] = None, - ) -> None: - """Add an item to an allow or denylist.""" - allow_type = "whitelist" if allowed else "blacklist" - - # If this is a server invite, we gotta validate it. - if list_type == "GUILD_INVITE": - log.trace(f"{content} is a guild invite, attempting to validate.") - validator = ValidDiscordServerInvite() - guild_data = await validator.convert(ctx, content) - - # If we make it this far without raising a BadArgument, the invite is - # valid. Let's convert the content to an ID. - log.trace(f"{content} validated as server invite. Converting to ID.") - content = guild_data.get("id") - - # Unless the user has specified another comment, let's - # use the server name as the comment so that the list - # of guild IDs will be more easily readable when we - # display it. - if not comment: - comment = guild_data.get("name") - - # Try to add the item to the database - log.trace(f"Trying to add the {content} item to the {list_type} {allow_type}") - payload = { - 'allowed': allowed, - 'type': list_type, - 'content': content, - 'comment': comment, - } - - try: - item = await self.bot.api_client.post( - "bot/allow_deny_lists", - json=payload - ) - except ResponseCodeError as e: - if e.status == 500: - await ctx.message.add_reaction("❌") - log.debug( - f"{ctx.author} tried to add data to a {allow_type}, but the API returned 500, " - "probably because the request violated the UniqueConstraint." - ) - raise BadArgument( - f"Unable to add the item to the {allow_type}. " - "The item probably already exists. Keep in mind that a " - "blacklist and a whitelist for the same item cannot co-exist, " - "and we do not permit any duplicates." - ) - raise - - # Insert the item into the cache - type_ = item.get("type") - allowed = item.get("allowed") - metadata = { - "content": item.get("content"), - "comment": item.get("comment"), - "id": item.get("id"), - "created_at": item.get("created_at"), - "updated_at": item.get("updated_at"), - } - self.bot.allow_deny_list_cache.setdefault(f"{type_}.{allowed}", []).append(metadata) - await ctx.message.add_reaction("✅") - - async def _delete_data(self, ctx: Context, allowed: bool, list_type: ValidAllowDenyListType, content: str) -> None: - """Remove an item from an allow or denylist.""" - item = None - allow_type = "whitelist" if allowed else "blacklist" - id_converter = IDConverter() - - # If this is a server invite, we need to convert it. - if list_type == "GUILD_INVITE" and not id_converter._get_id_match(content): - log.trace(f"{content} is a guild invite, attempting to validate.") - validator = ValidDiscordServerInvite() - guild_data = await validator.convert(ctx, content) - - # If we make it this far without raising a BadArgument, the invite is - # valid. Let's convert the content to an ID. - log.trace(f"{content} validated as server invite. Converting to ID.") - content = guild_data.get("id") - - # Find the content and delete it. - log.trace(f"Trying to delete the {content} item from the {list_type} {allow_type}") - for allow_list in self.bot.allow_deny_list_cache.get(f"{list_type}.{allowed}", []): - if content == allow_list.get("content"): - item = allow_list - break - - if item is not None: - await self.bot.api_client.delete( - f"bot/allow_deny_lists/{item.get('id')}" - ) - self.bot.allow_deny_list_cache[f"{list_type}.{allowed}"].remove(item) - await ctx.message.add_reaction("✅") - - async def _list_all_data(self, ctx: Context, allowed: bool, list_type: ValidAllowDenyListType) -> None: - """Paginate and display all items in an allow or denylist.""" - allow_type = "whitelist" if allowed else "blacklist" - result = self.bot.allow_deny_list_cache.get(f"{list_type}.{allowed}", []) - - # Build a list of lines we want to show in the paginator - lines = [] - for item in result: - line = f"• `{item.get('content')}`" - - if item.get("comment"): - line += f" - {item.get('comment')}" - - lines.append(line) - lines = sorted(lines) - - # Build the embed - list_type_plural = list_type.lower().replace("_", " ").title() + "s" - embed = Embed( - title=f"{allow_type.title()}ed {list_type_plural} ({len(result)} total)", - colour=Colour.blue() - ) - log.trace(f"Trying to list {len(result)} items from the {list_type.lower()} {allow_type}") - - if result: - await LinePaginator.paginate(lines, ctx, embed, max_lines=15, empty=False) - else: - embed.description = "Hmmm, seems like there's nothing here yet." - await ctx.send(embed=embed) - - @group(aliases=("allowlist", "allow", "al", "wl")) - async def whitelist(self, ctx: Context) -> None: - """Group for whitelisting commands.""" - if not ctx.invoked_subcommand: - await ctx.send_help(ctx.command) - - @group(aliases=("denylist", "deny", "bl", "dl")) - async def blacklist(self, ctx: Context) -> None: - """Group for blacklisting commands.""" - if not ctx.invoked_subcommand: - await ctx.send_help(ctx.command) - - @whitelist.command(name="add", aliases=("a", "set")) - async def allow_add( - self, - ctx: Context, - list_type: ValidAllowDenyListType, - content: str, - *, - comment: Optional[str] = None, - ) -> None: - """Add an item to the specified allowlist.""" - await self._add_data(ctx, True, list_type, content, comment) - - @blacklist.command(name="add", aliases=("a", "set")) - async def deny_add( - self, - ctx: Context, - list_type: ValidAllowDenyListType, - content: str, - *, - comment: Optional[str] = None, - ) -> None: - """Add an item to the specified denylist.""" - await self._add_data(ctx, False, list_type, content, comment) - - @whitelist.command(name="remove", aliases=("delete", "rm",)) - async def allow_delete(self, ctx: Context, list_type: ValidAllowDenyListType, content: str) -> None: - """Remove an item from the specified allowlist.""" - await self._delete_data(ctx, True, list_type, content) - - @blacklist.command(name="remove", aliases=("delete", "rm",)) - async def deny_delete(self, ctx: Context, list_type: ValidAllowDenyListType, content: str) -> None: - """Remove an item from the specified denylist.""" - await self._delete_data(ctx, False, list_type, content) - - @whitelist.command(name="get", aliases=("list", "ls", "fetch", "show")) - async def allow_get(self, ctx: Context, list_type: ValidAllowDenyListType) -> None: - """Get the contents of a specified allowlist.""" - await self._list_all_data(ctx, True, list_type) - - @blacklist.command(name="get", aliases=("list", "ls", "fetch", "show")) - async def deny_get(self, ctx: Context, list_type: ValidAllowDenyListType) -> None: - """Get the contents of a specified denylist.""" - await self._list_all_data(ctx, False, list_type) - - def cog_check(self, ctx: Context) -> bool: - """Only allow moderators to invoke the commands in this cog.""" - return with_role_check(ctx, *constants.MODERATION_ROLES) - - -def setup(bot: Bot) -> None: - """Load the AllowDenyLists cog.""" - bot.add_cog(AllowDenyLists(bot)) diff --git a/bot/cogs/antimalware.py b/bot/cogs/antimalware.py index 5b56f937f..9a100b3fc 100644 --- a/bot/cogs/antimalware.py +++ b/bot/cogs/antimalware.py @@ -40,7 +40,7 @@ class AntiMalware(Cog): def _get_whitelisted_file_formats(self) -> list: """Get the file formats currently on the whitelist.""" - return [item['content'] for item in self.bot.allow_deny_list_cache['file_format.True']] + return [item['content'] for item in self.bot.filter_list_cache['file_format.True']] def _get_disallowed_extensions(self, message: Message) -> t.Iterable[str]: """Get an iterable containing all the disallowed extensions of attachments.""" diff --git a/bot/cogs/filter_lists.py b/bot/cogs/filter_lists.py new file mode 100644 index 000000000..d1db9830e --- /dev/null +++ b/bot/cogs/filter_lists.py @@ -0,0 +1,218 @@ +import logging +from typing import Optional + +from discord import Colour, Embed +from discord.ext.commands import BadArgument, Cog, Context, IDConverter, group + +from bot import constants +from bot.api import ResponseCodeError +from bot.bot import Bot +from bot.converters import ValidDiscordServerInvite, ValidFilterListType +from bot.pagination import LinePaginator +from bot.utils.checks import with_role_check + +log = logging.getLogger(__name__) + + +class FilterLists(Cog): + """Commands for blacklisting and whitelisting things.""" + + def __init__(self, bot: Bot) -> None: + self.bot = bot + + async def _add_data( + self, + ctx: Context, + allowed: bool, + list_type: ValidFilterListType, + content: str, + comment: Optional[str] = None, + ) -> None: + """Add an item to a filterlist.""" + allow_type = "whitelist" if allowed else "blacklist" + + # If this is a server invite, we gotta validate it. + if list_type == "GUILD_INVITE": + log.trace(f"{content} is a guild invite, attempting to validate.") + validator = ValidDiscordServerInvite() + guild_data = await validator.convert(ctx, content) + + # If we make it this far without raising a BadArgument, the invite is + # valid. Let's convert the content to an ID. + log.trace(f"{content} validated as server invite. Converting to ID.") + content = guild_data.get("id") + + # Unless the user has specified another comment, let's + # use the server name as the comment so that the list + # of guild IDs will be more easily readable when we + # display it. + if not comment: + comment = guild_data.get("name") + + # Try to add the item to the database + log.trace(f"Trying to add the {content} item to the {list_type} {allow_type}") + payload = { + 'allowed': allowed, + 'type': list_type, + 'content': content, + 'comment': comment, + } + + try: + item = await self.bot.api_client.post( + "bot/filter-lists", + json=payload + ) + except ResponseCodeError as e: + if e.status == 500: + await ctx.message.add_reaction("❌") + log.debug( + f"{ctx.author} tried to add data to a {allow_type}, but the API returned 500, " + "probably because the request violated the UniqueConstraint." + ) + raise BadArgument( + f"Unable to add the item to the {allow_type}. " + "The item probably already exists. Keep in mind that a " + "blacklist and a whitelist for the same item cannot co-exist, " + "and we do not permit any duplicates." + ) + raise + + # Insert the item into the cache + type_ = item.get("type") + allowed = item.get("allowed") + metadata = { + "content": item.get("content"), + "comment": item.get("comment"), + "id": item.get("id"), + "created_at": item.get("created_at"), + "updated_at": item.get("updated_at"), + } + self.bot.filter_list_cache.setdefault(f"{type_}.{allowed}", []).append(metadata) + await ctx.message.add_reaction("✅") + + async def _delete_data(self, ctx: Context, allowed: bool, list_type: ValidFilterListType, content: str) -> None: + """Remove an item from a filterlist.""" + item = None + allow_type = "whitelist" if allowed else "blacklist" + id_converter = IDConverter() + + # If this is a server invite, we need to convert it. + if list_type == "GUILD_INVITE" and not id_converter._get_id_match(content): + log.trace(f"{content} is a guild invite, attempting to validate.") + validator = ValidDiscordServerInvite() + guild_data = await validator.convert(ctx, content) + + # If we make it this far without raising a BadArgument, the invite is + # valid. Let's convert the content to an ID. + log.trace(f"{content} validated as server invite. Converting to ID.") + content = guild_data.get("id") + + # Find the content and delete it. + log.trace(f"Trying to delete the {content} item from the {list_type} {allow_type}") + for allow_list in self.bot.filter_list_cache.get(f"{list_type}.{allowed}", []): + if content == allow_list.get("content"): + item = allow_list + break + + if item is not None: + await self.bot.api_client.delete( + f"bot/filter-lists/{item.get('id')}" + ) + self.bot.filter_list_cache[f"{list_type}.{allowed}"].remove(item) + await ctx.message.add_reaction("✅") + + async def _list_all_data(self, ctx: Context, allowed: bool, list_type: ValidFilterListType) -> None: + """Paginate and display all items in a filterlist.""" + allow_type = "whitelist" if allowed else "blacklist" + result = self.bot.filter_list_cache.get(f"{list_type}.{allowed}", []) + + # Build a list of lines we want to show in the paginator + lines = [] + for item in result: + line = f"• `{item.get('content')}`" + + if item.get("comment"): + line += f" - {item.get('comment')}" + + lines.append(line) + lines = sorted(lines) + + # Build the embed + list_type_plural = list_type.lower().replace("_", " ").title() + "s" + embed = Embed( + title=f"{allow_type.title()}ed {list_type_plural} ({len(result)} total)", + colour=Colour.blue() + ) + log.trace(f"Trying to list {len(result)} items from the {list_type.lower()} {allow_type}") + + if result: + await LinePaginator.paginate(lines, ctx, embed, max_lines=15, empty=False) + else: + embed.description = "Hmmm, seems like there's nothing here yet." + await ctx.send(embed=embed) + + @group(aliases=("allowlist", "allow", "al", "wl")) + async def whitelist(self, ctx: Context) -> None: + """Group for whitelisting commands.""" + if not ctx.invoked_subcommand: + await ctx.send_help(ctx.command) + + @group(aliases=("denylist", "deny", "bl", "dl")) + async def blacklist(self, ctx: Context) -> None: + """Group for blacklisting commands.""" + if not ctx.invoked_subcommand: + await ctx.send_help(ctx.command) + + @whitelist.command(name="add", aliases=("a", "set")) + async def allow_add( + self, + ctx: Context, + list_type: ValidFilterListType, + content: str, + *, + comment: Optional[str] = None, + ) -> None: + """Add an item to the specified allowlist.""" + await self._add_data(ctx, True, list_type, content, comment) + + @blacklist.command(name="add", aliases=("a", "set")) + async def deny_add( + self, + ctx: Context, + list_type: ValidFilterListType, + content: str, + *, + comment: Optional[str] = None, + ) -> None: + """Add an item to the specified denylist.""" + await self._add_data(ctx, False, list_type, content, comment) + + @whitelist.command(name="remove", aliases=("delete", "rm",)) + async def allow_delete(self, ctx: Context, list_type: ValidFilterListType, content: str) -> None: + """Remove an item from the specified allowlist.""" + await self._delete_data(ctx, True, list_type, content) + + @blacklist.command(name="remove", aliases=("delete", "rm",)) + async def deny_delete(self, ctx: Context, list_type: ValidFilterListType, content: str) -> None: + """Remove an item from the specified denylist.""" + await self._delete_data(ctx, False, list_type, content) + + @whitelist.command(name="get", aliases=("list", "ls", "fetch", "show")) + async def allow_get(self, ctx: Context, list_type: ValidFilterListType) -> None: + """Get the contents of a specified allowlist.""" + await self._list_all_data(ctx, True, list_type) + + @blacklist.command(name="get", aliases=("list", "ls", "fetch", "show")) + async def deny_get(self, ctx: Context, list_type: ValidFilterListType) -> None: + """Get the contents of a specified denylist.""" + await self._list_all_data(ctx, False, list_type) + + def cog_check(self, ctx: Context) -> bool: + """Only allow moderators to invoke the commands in this cog.""" + return with_role_check(ctx, *constants.MODERATION_ROLES) + + +def setup(bot: Bot) -> None: + """Load the FilterLists cog.""" + bot.add_cog(FilterLists(bot)) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 8897cbaf9..652af5ff5 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -99,9 +99,9 @@ class Filtering(Cog): self.bot.loop.create_task(self.reschedule_offensive_msg_deletion()) - def _get_allowlist_items(self, list_type: str, *, allowed: bool, compiled: Optional[bool] = False) -> list: - """Fetch items from the allow_deny_list_cache.""" - items = self.bot.allow_deny_list_cache.get(f"{list_type.upper()}.{allowed}", []) + def _get_filterlist_items(self, list_type: str, *, allowed: bool, compiled: Optional[bool] = False) -> list: + """Fetch items from the filter_list_cache.""" + items = self.bot.filter_list_cache.get(f"{list_type.upper()}.{allowed}", []) if compiled: return [re.compile(fr'{item["content"]}', flags=re.IGNORECASE) for item in items] @@ -143,7 +143,7 @@ class Filtering(Cog): def get_name_matches(self, name: str) -> List[re.Match]: """Check bad words from passed string (name). Return list of matches.""" matches = [] - watchlist_patterns = self._get_allowlist_items('word_watchlist', allowed=False, compiled=True) + watchlist_patterns = self._get_filterlist_items('word_watchlist', allowed=False, compiled=True) for pattern in watchlist_patterns: if match := pattern.search(name): matches.append(match) @@ -408,7 +408,7 @@ class Filtering(Cog): if URL_RE.search(text): return False - watchlist_patterns = self._get_allowlist_items('word_watchlist', allowed=False, compiled=True) + watchlist_patterns = self._get_filterlist_items('word_watchlist', allowed=False, compiled=True) for pattern in watchlist_patterns: match = pattern.search(text) if match: @@ -420,7 +420,7 @@ class Filtering(Cog): return False text = text.lower() - domain_blacklist = self._get_allowlist_items("domain_name", allowed=False) + domain_blacklist = self._get_filterlist_items("domain_name", allowed=False) for url in domain_blacklist: if url.lower() in text: @@ -468,8 +468,8 @@ class Filtering(Cog): return True guild_id = guild.get("id") - guild_invite_whitelist = self._get_allowlist_items("guild_invite", allowed=True) - guild_invite_blacklist = self._get_allowlist_items("guild_invite", allowed=False) + guild_invite_whitelist = self._get_filterlist_items("guild_invite", allowed=True) + guild_invite_blacklist = self._get_filterlist_items("guild_invite", allowed=False) # Is this invite allowed? guild_partnered_or_verified = ( diff --git a/bot/converters.py b/bot/converters.py index 41cd3f3e5..158bf1a16 100644 --- a/bot/converters.py +++ b/bot/converters.py @@ -72,18 +72,18 @@ class ValidDiscordServerInvite(Converter): raise BadArgument("This does not appear to be a valid Discord server invite.") -class ValidAllowDenyListType(Converter): +class ValidFilterListType(Converter): """ - A converter that checks whether the given string is a valid AllowDenyList type. + A converter that checks whether the given string is a valid FilterList type. - Raises `BadArgument` if the argument is not a valid AllowDenyList type, and simply + Raises `BadArgument` if the argument is not a valid FilterList type, and simply passes through the given argument otherwise. """ async def convert(self, ctx: Context, list_type: str) -> str: - """Checks whether the given string is a valid AllowDenyList type.""" + """Checks whether the given string is a valid FilterList type.""" try: - valid_types = await ctx.bot.api_client.get('bot/allow_deny_lists/get_types') + valid_types = await ctx.bot.api_client.get('bot/filter-lists/get-types') except ResponseCodeError: raise BadArgument("Cannot validate list_type: Unable to fetch valid types from API.") diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 1e010d2ce..664fa8f19 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -14,7 +14,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): def setUp(self): """Sets up fresh objects for each test.""" self.bot = MockBot() - self.bot.allow_deny_list_cache = { + self.bot.filter_list_cache = { "file_format.True": [ {"content": ".first"}, {"content": ".second"}, -- cgit v1.2.3 From e0837f4f6dd7c5c2d6fc0811dccfaf1ecae768ba Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 29 Jul 2020 20:14:52 +0200 Subject: Restructure bot.filter_list_cache. This is an optimization designed to eliminate all the list comprehensions we were doing inside antimalware and filtering. The cache is now structured so that the content is the key and the metadata is the value. --- bot/bot.py | 8 ++++---- bot/cogs/antimalware.py | 2 +- bot/cogs/filter_lists.py | 18 +++++++++--------- bot/cogs/filtering.py | 3 +-- tests/bot/cogs/test_antimalware.py | 10 +++++----- 5 files changed, 20 insertions(+), 21 deletions(-) (limited to 'tests') diff --git a/bot/bot.py b/bot/bot.py index 5deb986ec..4492feaa9 100644 --- a/bot/bot.py +++ b/bot/bot.py @@ -35,7 +35,7 @@ class Bot(commands.Bot): self.redis_ready = asyncio.Event() self.redis_closed = False self.api_client = api.APIClient(loop=self.loop) - self.filter_list_cache = defaultdict(list) + self.filter_list_cache = defaultdict(dict) self._connector = None self._resolver = None @@ -169,14 +169,14 @@ class Bot(commands.Bot): """Add an item to the bots filter_list_cache.""" type_ = item["type"] allowed = item["allowed"] - metadata = { + content = item["content"] + + self.filter_list_cache[f"{type_}.{allowed}"][content] = { "id": item["id"], - "content": item["content"], "comment": item["comment"], "created_at": item["created_at"], "updated_at": item["updated_at"], } - self.filter_list_cache[f"{type_}.{allowed}"].append(metadata) async def login(self, *args, **kwargs) -> None: """Re-create the connector and set up sessions before logging into Discord.""" diff --git a/bot/cogs/antimalware.py b/bot/cogs/antimalware.py index 9a100b3fc..c76bd2c60 100644 --- a/bot/cogs/antimalware.py +++ b/bot/cogs/antimalware.py @@ -40,7 +40,7 @@ class AntiMalware(Cog): def _get_whitelisted_file_formats(self) -> list: """Get the file formats currently on the whitelist.""" - return [item['content'] for item in self.bot.filter_list_cache['file_format.True']] + return self.bot.filter_list_cache['FILE_FORMAT.True'].keys() def _get_disallowed_extensions(self, message: Message) -> t.Iterable[str]: """Get an iterable containing all the disallowed extensions of attachments.""" diff --git a/bot/cogs/filter_lists.py b/bot/cogs/filter_lists.py index a93de2de9..3331be014 100644 --- a/bot/cogs/filter_lists.py +++ b/bot/cogs/filter_lists.py @@ -88,16 +88,16 @@ class FilterLists(Cog): # Find the content and delete it. log.trace(f"Trying to delete the {content} item from the {list_type} {allow_type}") - for allow_list in self.bot.filter_list_cache[f"{list_type}.{allowed}"]: - if content == allow_list.get("content"): - item = allow_list + for allow_list, metadata in self.bot.filter_list_cache[f"{list_type}.{allowed}"].items(): + if content == allow_list: + item = metadata break if item is not None: await self.bot.api_client.delete( - f"bot/filter-lists/{item.get('id')}" + f"bot/filter-lists/{item['id']}" ) - self.bot.filter_list_cache[f"{list_type}.{allowed}"].remove(item) + del self.bot.filter_list_cache[f"{list_type}.{allowed}"][content] await ctx.message.add_reaction("✅") async def _list_all_data(self, ctx: Context, allowed: bool, list_type: ValidFilterListType) -> None: @@ -107,11 +107,11 @@ class FilterLists(Cog): # Build a list of lines we want to show in the paginator lines = [] - for item in result: - line = f"• `{item.get('content')}`" + for content, metadata in result.items(): + line = f"• `{content}`" - if item.get("comment"): - line += f" - {item.get('comment')}" + if metadata.get("comment"): + line += f" - {metadata.get('comment')}" lines.append(line) lines = sorted(lines) diff --git a/bot/cogs/filtering.py b/bot/cogs/filtering.py index 7787d396d..0951cb740 100644 --- a/bot/cogs/filtering.py +++ b/bot/cogs/filtering.py @@ -101,8 +101,7 @@ class Filtering(Cog): def _get_filterlist_items(self, list_type: str, *, allowed: bool) -> list: """Fetch items from the filter_list_cache.""" - items = self.bot.filter_list_cache[f"{list_type.upper()}.{allowed}"] - return [item["content"] for item in items] + return self.bot.filter_list_cache[f"{list_type.upper()}.{allowed}"].keys() @staticmethod def _expand_spoilers(text: str) -> str: diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index 664fa8f19..82eadf226 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -15,11 +15,11 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): """Sets up fresh objects for each test.""" self.bot = MockBot() self.bot.filter_list_cache = { - "file_format.True": [ - {"content": ".first"}, - {"content": ".second"}, - {"content": ".third"} - ] + "file_format.True": { + ".first": {}, + ".second": {}, + ".third": {}, + } } self.cog = antimalware.AntiMalware(self.bot) self.message = MockMessage() -- cgit v1.2.3 From 0cfc918c6d68764c380f1188f3bc5508e6b27030 Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Wed, 29 Jul 2020 20:24:06 +0200 Subject: Fix broken antimalware tests. --- 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 82eadf226..ecb7abf00 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -15,7 +15,7 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): """Sets up fresh objects for each test.""" self.bot = MockBot() self.bot.filter_list_cache = { - "file_format.True": { + "FILE_FORMAT.True": { ".first": {}, ".second": {}, ".third": {}, -- cgit v1.2.3 From ed4ebbf5f7ee751f87554831e277d270cf36ac40 Mon Sep 17 00:00:00 2001 From: Joseph Banks Date: Fri, 14 Aug 2020 21:19:04 +0100 Subject: Update tests for user commands --- tests/bot/cogs/test_information.py | 87 ++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 32 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_information.py b/tests/bot/cogs/test_information.py index 79c0e0ad3..77b0ddf17 100644 --- a/tests/bot/cogs/test_information.py +++ b/tests/bot/cogs/test_information.py @@ -215,10 +215,10 @@ class UserInfractionHelperMethodTests(unittest.TestCase): with self.subTest(method=method, api_response=api_response, expected_lines=expected_lines): self.bot.api_client.get.return_value = api_response - expected_output = "\n".join(default_header + expected_lines) + expected_output = "\n".join(expected_lines) actual_output = asyncio.run(method(self.member)) - self.assertEqual(expected_output, actual_output) + self.assertEqual((default_header, expected_output), actual_output) def test_basic_user_infraction_counts_returns_correct_strings(self): """The method should correctly list both the total and active number of non-hidden infractions.""" @@ -249,7 +249,7 @@ class UserInfractionHelperMethodTests(unittest.TestCase): }, ) - header = ["**Infractions**"] + header = "Infractions" self._method_subtests(self.cog.basic_user_infraction_counts, test_values, header) @@ -258,7 +258,7 @@ class UserInfractionHelperMethodTests(unittest.TestCase): test_values = ( { "api response": [], - "expected_lines": ["This user has never received an infraction."], + "expected_lines": ["No infractions"], }, # Shows non-hidden inactive infraction as expected { @@ -304,7 +304,7 @@ class UserInfractionHelperMethodTests(unittest.TestCase): }, ) - header = ["**Infractions**"] + header = "Infractions" self._method_subtests(self.cog.expanded_user_infraction_counts, test_values, header) @@ -313,15 +313,15 @@ class UserInfractionHelperMethodTests(unittest.TestCase): test_values = ( { "api response": [], - "expected_lines": ["This user has never been nominated."], + "expected_lines": ["No nominations"], }, { "api response": [{'active': True}], - "expected_lines": ["This user is **currently** nominated (1 nomination in total)."], + "expected_lines": ["This user is **currently** nominated", "(1 nomination in total)"], }, { "api response": [{'active': True}, {'active': False}], - "expected_lines": ["This user is **currently** nominated (2 nominations in total)."], + "expected_lines": ["This user is **currently** nominated", "(2 nominations in total)"], }, { "api response": [{'active': False}], @@ -334,7 +334,7 @@ class UserInfractionHelperMethodTests(unittest.TestCase): ) - header = ["**Nominations**"] + header = "Nominations" self._method_subtests(self.cog.user_nomination_counts, test_values, header) @@ -350,7 +350,10 @@ class UserEmbedTests(unittest.TestCase): self.bot.api_client.get = unittest.mock.AsyncMock() self.cog = information.Information(self.bot) - @unittest.mock.patch(f"{COG_PATH}.basic_user_infraction_counts", new=unittest.mock.AsyncMock(return_value="")) + @unittest.mock.patch( + f"{COG_PATH}.basic_user_infraction_counts", + new=unittest.mock.AsyncMock(return_value=("Infractions", "basic infractions")) + ) def test_create_user_embed_uses_string_representation_of_user_in_title_if_nick_is_not_available(self): """The embed should use the string representation of the user if they don't have a nick.""" ctx = helpers.MockContext(channel=helpers.MockTextChannel(id=1)) @@ -362,7 +365,10 @@ class UserEmbedTests(unittest.TestCase): self.assertEqual(embed.title, "Mr. Hemlock") - @unittest.mock.patch(f"{COG_PATH}.basic_user_infraction_counts", new=unittest.mock.AsyncMock(return_value="")) + @unittest.mock.patch( + f"{COG_PATH}.basic_user_infraction_counts", + new=unittest.mock.AsyncMock(return_value=("Infractions", "basic infractions")) + ) def test_create_user_embed_uses_nick_in_title_if_available(self): """The embed should use the nick if it's available.""" ctx = helpers.MockContext(channel=helpers.MockTextChannel(id=1)) @@ -374,7 +380,10 @@ class UserEmbedTests(unittest.TestCase): self.assertEqual(embed.title, "Cat lover (Mr. Hemlock)") - @unittest.mock.patch(f"{COG_PATH}.basic_user_infraction_counts", new=unittest.mock.AsyncMock(return_value="")) + @unittest.mock.patch( + f"{COG_PATH}.basic_user_infraction_counts", + new=unittest.mock.AsyncMock(return_value=("Infractions", "basic infractions")) + ) def test_create_user_embed_ignores_everyone_role(self): """Created `!user` embeds should not contain mention of the @everyone-role.""" ctx = helpers.MockContext(channel=helpers.MockTextChannel(id=1)) @@ -386,8 +395,8 @@ class UserEmbedTests(unittest.TestCase): embed = asyncio.run(self.cog.create_user_embed(ctx, user)) - self.assertIn("&Admins", embed.description) - self.assertNotIn("&Everyone", embed.description) + self.assertIn("&Admins", embed.fields[1].value) + self.assertNotIn("&Everyone", embed.fields[1].value) @unittest.mock.patch(f"{COG_PATH}.expanded_user_infraction_counts", new_callable=unittest.mock.AsyncMock) @unittest.mock.patch(f"{COG_PATH}.user_nomination_counts", new_callable=unittest.mock.AsyncMock) @@ -398,8 +407,8 @@ class UserEmbedTests(unittest.TestCase): moderators_role = helpers.MockRole(name='Moderators') moderators_role.colour = 100 - infraction_counts.return_value = "expanded infractions info" - nomination_counts.return_value = "nomination info" + infraction_counts.return_value = ("Infractions", "expanded infractions info") + nomination_counts.return_value = ("Nominations", "nomination info") user = helpers.MockMember(id=314, roles=[moderators_role], top_role=moderators_role) embed = asyncio.run(self.cog.create_user_embed(ctx, user)) @@ -409,20 +418,19 @@ class UserEmbedTests(unittest.TestCase): self.assertEqual( textwrap.dedent(f""" - **User Information** Created: {"1 year ago"} Profile: {user.mention} ID: {user.id} + """).strip(), + embed.fields[0].value + ) - **Member Information** + self.assertEqual( + textwrap.dedent(f""" Joined: {"1 year ago"} Roles: &Moderators - - expanded infractions info - - nomination info """).strip(), - embed.description + embed.fields[1].value ) @unittest.mock.patch(f"{COG_PATH}.basic_user_infraction_counts", new_callable=unittest.mock.AsyncMock) @@ -433,7 +441,7 @@ class UserEmbedTests(unittest.TestCase): moderators_role = helpers.MockRole(name='Moderators') moderators_role.colour = 100 - infraction_counts.return_value = "basic infractions info" + infraction_counts.return_value = ("Infractions", "basic infractions info") user = helpers.MockMember(id=314, roles=[moderators_role], top_role=moderators_role) embed = asyncio.run(self.cog.create_user_embed(ctx, user)) @@ -442,21 +450,30 @@ class UserEmbedTests(unittest.TestCase): self.assertEqual( textwrap.dedent(f""" - **User Information** Created: {"1 year ago"} Profile: {user.mention} ID: {user.id} + """).strip(), + embed.fields[0].value + ) - **Member Information** + self.assertEqual( + textwrap.dedent(f""" Joined: {"1 year ago"} Roles: &Moderators - - basic infractions info """).strip(), - embed.description + embed.fields[1].value + ) + + self.assertEqual( + "basic infractions info", + embed.fields[3].value ) - @unittest.mock.patch(f"{COG_PATH}.basic_user_infraction_counts", new=unittest.mock.AsyncMock(return_value="")) + @unittest.mock.patch( + f"{COG_PATH}.basic_user_infraction_counts", + new=unittest.mock.AsyncMock(return_value=("Infractions", "basic infractions")) + ) def test_create_user_embed_uses_top_role_colour_when_user_has_roles(self): """The embed should be created with the colour of the top role, if a top role is available.""" ctx = helpers.MockContext() @@ -469,7 +486,10 @@ class UserEmbedTests(unittest.TestCase): self.assertEqual(embed.colour, discord.Colour(moderators_role.colour)) - @unittest.mock.patch(f"{COG_PATH}.basic_user_infraction_counts", new=unittest.mock.AsyncMock(return_value="")) + @unittest.mock.patch( + f"{COG_PATH}.basic_user_infraction_counts", + new=unittest.mock.AsyncMock(return_value=("Infractions", "basic infractions")) + ) def test_create_user_embed_uses_blurple_colour_when_user_has_no_roles(self): """The embed should be created with a blurple colour if the user has no assigned roles.""" ctx = helpers.MockContext() @@ -479,7 +499,10 @@ class UserEmbedTests(unittest.TestCase): self.assertEqual(embed.colour, discord.Colour.blurple()) - @unittest.mock.patch(f"{COG_PATH}.basic_user_infraction_counts", new=unittest.mock.AsyncMock(return_value="")) + @unittest.mock.patch( + f"{COG_PATH}.basic_user_infraction_counts", + new=unittest.mock.AsyncMock(return_value=("Infractions", "basic infractions")) + ) def test_create_user_embed_uses_png_format_of_user_avatar_as_thumbnail(self): """The embed thumbnail should be set to the user's avatar in `png` format.""" ctx = helpers.MockContext() -- cgit v1.2.3 From 520ac0f9871bf6775d76eea753ed2a940704e92d Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sat, 22 Aug 2020 20:44:48 -0700 Subject: Include root aliases in the command name conflict test --- tests/bot/cogs/test_cogs.py | 1 + 1 file changed, 1 insertion(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_cogs.py b/tests/bot/cogs/test_cogs.py index fdda59a8f..30a04422a 100644 --- a/tests/bot/cogs/test_cogs.py +++ b/tests/bot/cogs/test_cogs.py @@ -53,6 +53,7 @@ class CommandNameTests(unittest.TestCase): """Return a list of all qualified names, including aliases, for the `command`.""" names = [f"{command.full_parent_name} {alias}".strip() for alias in command.aliases] names.append(command.qualified_name) + names += getattr(command, "root_aliases", []) return names -- cgit v1.2.3 From b7644aa822def549e2591b53c69af3cf44355ac9 Mon Sep 17 00:00:00 2001 From: Xithrius Date: Mon, 31 Aug 2020 19:56:24 -0700 Subject: Removed ImagePaginator testing. --- tests/bot/test_pagination.py | 15 --------------- 1 file changed, 15 deletions(-) (limited to 'tests') diff --git a/tests/bot/test_pagination.py b/tests/bot/test_pagination.py index ce880d457..630f2516d 100644 --- a/tests/bot/test_pagination.py +++ b/tests/bot/test_pagination.py @@ -44,18 +44,3 @@ class LinePaginatorTests(TestCase): self.paginator.add_line('x' * (self.paginator.scale_to_size + 1)) # Note: item at index 1 is the truncated line, index 0 is prefix self.assertEqual(self.paginator._current_page[1], 'x' * self.paginator.scale_to_size) - - -class ImagePaginatorTests(TestCase): - """Tests functionality of the `ImagePaginator`.""" - - def setUp(self): - """Create a paginator for the test method.""" - self.paginator = pagination.ImagePaginator() - - def test_add_image_appends_image(self): - """`add_image` appends the image to the image list.""" - image = 'lemon' - self.paginator.add_image(image) - - assert self.paginator.images == [image] -- cgit v1.2.3 From 1a47f5d80f2f91c3da5a9626e9a6694381d49cd0 Mon Sep 17 00:00:00 2001 From: wookie184 Date: Tue, 1 Sep 2020 12:22:43 +0100 Subject: Fixed old tests and added 2 new ones --- tests/bot/cogs/test_antimalware.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_antimalware.py b/tests/bot/cogs/test_antimalware.py index ecb7abf00..f50c0492d 100644 --- a/tests/bot/cogs/test_antimalware.py +++ b/tests/bot/cogs/test_antimalware.py @@ -23,6 +23,8 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): } self.cog = antimalware.AntiMalware(self.bot) self.message = MockMessage() + self.message.webhook_id = None + self.message.author.bot = None self.whitelist = [".first", ".second", ".third"] async def test_message_with_allowed_attachment(self): @@ -48,6 +50,26 @@ class AntiMalwareCogTests(unittest.IsolatedAsyncioTestCase): self.message.delete.assert_not_called() + async def test_webhook_message_with_illegal_extension(self): + """A webhook message containing an illegal extension should be ignored.""" + attachment = MockAttachment(filename="python.disallowed") + self.message.webhook_id = 697140105563078727 + self.message.attachments = [attachment] + + await self.cog.on_message(self.message) + + self.message.delete.assert_not_called() + + async def test_bot_message_with_illegal_extension(self): + """A bot message containing an illegal extension should be ignored.""" + attachment = MockAttachment(filename="python.disallowed") + self.message.author.bot = 409107086526644234 + self.message.attachments = [attachment] + + await self.cog.on_message(self.message) + + self.message.delete.assert_not_called() + async def test_message_with_illegal_extension_gets_deleted(self): """A message containing an illegal extension should send an embed.""" attachment = MockAttachment(filename="python.disallowed") -- cgit v1.2.3