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 d193a93828582965eb361dc6f3185291fff649a7 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:11:39 -0700 Subject: Test on_message_edit of token remover uses on_message --- tests/bot/cogs/test_token_remover.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 33d1ec170..e7b5a9bea 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,6 +1,7 @@ import asyncio import logging import unittest +from unittest import mock from unittest.mock import AsyncMock, MagicMock from discord import Colour @@ -14,7 +15,7 @@ from bot.constants import Channels, Colours, Event, Icons from tests.helpers import MockBot, MockMessage -class TokenRemoverTests(unittest.TestCase): +class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): """Tests the `TokenRemover` cog.""" def setUp(self): @@ -58,6 +59,13 @@ class TokenRemoverTests(unittest.TestCase): self.assertEqual(self.cog.mod_log, self.bot.get_cog.return_value) self.bot.get_cog.assert_called_once_with('ModLog') + async def test_on_message_edit_uses_on_message(self): + """The edit listener should delegate handling of the message to the normal listener.""" + self.cog.on_message = mock.create_autospec(self.cog.on_message, spec_set=True) + + await self.cog.on_message_edit(MockMessage(), self.msg) + self.cog.on_message.assert_awaited_once_with(self.msg) + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True @@ -77,7 +85,7 @@ class TokenRemoverTests(unittest.TestCase): for content in ('foo.bar.baz', 'x.y.'): with self.subTest(content=content): self.msg.content = content - coroutine = self.cog.on_message(self.msg) + coroutine = self.cog.is_maybe_token(self.msg) self.assertIsNone(asyncio.run(coroutine)) def test_censors_valid_tokens(self): -- cgit v1.2.3 From 0bfd003dbfc5919220129f984dc043421e535f8c Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:38:12 -0700 Subject: Add a test helper function to patch multiple attributes with autospecs This helper reduces redundancy/boilerplate by setting default values. It also has the consequence of shortening the length of the invocation, which makes it faster to use and easier to read. --- tests/helpers.py | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 2b79a6c2a..d444cc49d 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -23,6 +23,15 @@ for logger in logging.Logger.manager.loggerDict.values(): logger.setLevel(logging.CRITICAL) +def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: + """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" + # Caller's kwargs should take priority and overwrite the defaults. + kwargs = {'spec_set': True, 'autospec': True, **kwargs} + attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} + + return unittest.mock.patch.multiple(target, **attributes, **kwargs) + + class HashableMixin(discord.mixins.EqualityComparable): """ Mixin that provides similar hashing and equality functionality as discord.py's `Hashable` mixin. -- cgit v1.2.3 From e8bd69a6c556d78eca1a1eb2adfa26248273a1cd Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:42:07 -0700 Subject: Test token remover takes action if a token is found --- tests/bot/cogs/test_token_remover.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e7b5a9bea..e0ec67684 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -12,7 +12,7 @@ from bot.cogs.token_remover import ( setup as setup_cog, ) from bot.constants import Channels, Colours, Event, Icons -from tests.helpers import MockBot, MockMessage +from tests.helpers import MockBot, MockMessage, autospec class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @@ -66,6 +66,18 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): await self.cog.on_message_edit(MockMessage(), self.msg) self.cog.on_message.assert_awaited_once_with(self.msg) + @autospec(TokenRemover, "find_token_in_message", "take_action") + async def test_on_message_takes_action(self, find_token_in_message, take_action): + """Should take action if a valid token is found when a message is sent.""" + cog = TokenRemover(self.bot) + found_token = "foobar" + find_token_in_message.return_value = found_token + + await cog.on_message(self.msg) + + find_token_in_message.assert_called_once_with(self.msg) + take_action.assert_awaited_once_with(cog, self.msg, found_token) + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True -- cgit v1.2.3 From 4cf7996a1d4630ccb05f57569ca62b1798dc7a93 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 14:44:54 -0700 Subject: Test token remover skips messages without tokens --- tests/bot/cogs/test_token_remover.py | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e0ec67684..2b377e221 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -78,6 +78,17 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): find_token_in_message.assert_called_once_with(self.msg) take_action.assert_awaited_once_with(cog, self.msg, found_token) + @autospec(TokenRemover, "find_token_in_message", "take_action") + async def test_on_message_skips_missing_token(self, find_token_in_message, take_action): + """Shouldn't take action if a valid token isn't found when a message is sent.""" + cog = TokenRemover(self.bot) + find_token_in_message.return_value = False + + await cog.on_message(self.msg) + + find_token_in_message.assert_called_once_with(self.msg) + take_action.assert_not_awaited() + def test_ignores_bot_messages(self): """When the message event handler is called with a bot message, nothing is done.""" self.msg.author.bot = True -- cgit v1.2.3 From 593e09299c6e4115d41bfd5b074785a5e304a8d0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:41:14 -0700 Subject: Allow using arbitrary parameter names with the autospec decorator This gives the caller more flexibility. Sometimes attribute names are too long or they don't follow a naming scheme accepted by the linter. --- tests/helpers.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index d444cc49d..1ab8b455f 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -24,12 +24,25 @@ for logger in logging.Logger.manager.loggerDict.values(): def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: - """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" + """ + Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True. + + To allow for arbitrary parameter names to be used by the decorated function, the patchers have + no attribute names associated with them. As a consequence, it will not be possible to retrieve + mocks by their attribute names when using this as a context manager, + """ # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} - return unittest.mock.patch.multiple(target, **attributes, **kwargs) + patcher = unittest.mock.patch.multiple(target, **attributes, **kwargs) + + # Unset attribute names to allow arbitrary parameter names for the decorator function. + patcher.attribute_name = None + for additional_patcher in patcher.additional_patchers: + additional_patcher.attribute_name = None + + return patcher class HashableMixin(discord.mixins.EqualityComparable): -- cgit v1.2.3 From b0dd290710799c342240d066abaebbe9e6940b54 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:09:22 -0700 Subject: Fix test for token remover ignoring bot messages It's not possible to test this via asserting the return value of `on_message` since it never returns anything. Instead, the actual relevant unit, `find_token_in_message,` should be tested. --- tests/bot/cogs/test_token_remover.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2b377e221..e8b641101 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -89,11 +89,16 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): find_token_in_message.assert_called_once_with(self.msg) take_action.assert_not_awaited() - def test_ignores_bot_messages(self): - """When the message event handler is called with a bot message, nothing is done.""" + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_ignores_bot_messages(self, token_re): + """The token finder should ignore messages authored by bots.""" + cog = TokenRemover(self.bot) self.msg.author.bot = True - coroutine = self.cog.on_message(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + + return_value = cog.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.findall.assert_not_called() def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" -- cgit v1.2.3 From 52f0f8a29d7f239c961beaa81881bf4b09da4749 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 15:53:06 -0700 Subject: Test `find_token_in_message` returns None if no matches found --- tests/bot/cogs/test_token_remover.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index e8b641101..5932cf4f0 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -100,6 +100,20 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.findall.assert_not_called() + @autospec(TokenRemover, "is_maybe_token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): + """None should be returned if the regex matches no tokens in a message.""" + cog = TokenRemover(self.bot) + token_re.findall.return_value = () + self.msg.content = "foobar" + + return_value = cog.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.findall.assert_called_once_with(self.msg.content) + is_maybe_token.assert_not_called() + def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" for content in ('', 'lemon wins'): -- cgit v1.2.3 From cf658bd58559b2683527443f2908257f197ef0bb Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 16:06:47 -0700 Subject: Test `find_token_in_message` returns the found token --- tests/bot/cogs/test_token_remover.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5932cf4f0..2b946778b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -114,6 +114,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): token_re.findall.assert_called_once_with(self.msg.content) is_maybe_token.assert_not_called() + @autospec(TokenRemover, "is_maybe_token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_returns_found_token(self, token_re, is_maybe_token): + """The found token should be returned.""" + true_index = 1 + matches = ("foo", "bar", "baz") + side_effects = [False] * len(matches) + side_effects[true_index] = True + + cog = TokenRemover(self.bot) + self.msg.content = "foobar" + token_re.findall.return_value = matches + is_maybe_token.side_effect = side_effects + + return_value = cog.find_token_in_message(self.msg) + + self.assertEqual(return_value, matches[true_index]) + token_re.findall.assert_called_once_with(self.msg.content) + + # assert_has_calls isn't used cause it'd allow for extra calls before or after. + # The function should short-circuit, so nothing past true_index should have been used. + calls = [mock.call(match) for match in matches[:true_index + 1]] + self.assertEqual(is_maybe_token.mock_calls, calls) + def test_ignores_messages_without_tokens(self): """Messages without anything looking like a token are ignored.""" for content in ('', 'lemon wins'): -- cgit v1.2.3 From f92bc80d6bddb5c57c190187adaa528ae44536f6 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 16:25:14 -0700 Subject: Test token regex doesn't match invalid tokens --- tests/bot/cogs/test_token_remover.py | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2b946778b..b67602eb9 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -8,6 +8,7 @@ from discord import Colour from bot.cogs.token_remover import ( DELETION_MESSAGE_TEMPLATE, + TOKEN_RE, TokenRemover, setup as setup_cog, ) @@ -138,13 +139,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): calls = [mock.call(match) for match in matches[:true_index + 1]] self.assertEqual(is_maybe_token.mock_calls, calls) - def test_ignores_messages_without_tokens(self): - """Messages without anything looking like a token are ignored.""" - for content in ('', 'lemon wins'): - with self.subTest(content=content): - self.msg.content = content - coroutine = self.cog.on_message(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + def test_regex_invalid_tokens(self): + """Messages without anything looking like a token are not matched.""" + tokens = ( + "", + "lemon wins", + "..", + "x.y", + "x.y.", + ".y.z", + ".y.", + "..z", + "x..z", + " . . ", + "\n.\n.\n", + "'.'.'", + '"."."', + "(.(.(", + ").).)" + ) + + for token in tokens: + with self.subTest(token=token): + results = TOKEN_RE.findall(token) + self.assertEquals(len(results), 0) def test_ignores_messages_with_invalid_tokens(self): """Messages with values that are invalid tokens are ignored.""" -- cgit v1.2.3 From 34b836a8eba0f006c77a7b3f48f7ab14c37d31ee Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 17:47:09 -0700 Subject: Fix autospec decorator when used with multiple attributes The original approach of messing with the `attribute_name` didn't work for reasons I won't discuss here (would require knowledge of patcher internals). The new approach doesn't use patch.multiple but mimics it by applying multiple patch decorators to the function. As a consequence, this can no longer be used as a context manager. --- tests/helpers.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index 1ab8b455f..dfbe539ec 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -24,25 +24,21 @@ for logger in logging.Logger.manager.loggerDict.values(): def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: - """ - Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True. - - To allow for arbitrary parameter names to be used by the decorated function, the patchers have - no attribute names associated with them. As a consequence, it will not be possible to retrieve - mocks by their attribute names when using this as a context manager, - """ + """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} - attributes = {attribute: unittest.mock.DEFAULT for attribute in attributes} - - patcher = unittest.mock.patch.multiple(target, **attributes, **kwargs) - - # Unset attribute names to allow arbitrary parameter names for the decorator function. - patcher.attribute_name = None - for additional_patcher in patcher.additional_patchers: - additional_patcher.attribute_name = None - return patcher + # Import the target if it's a string. + # This is to support both object and string targets like patch.multiple. + if type(target) is str: + target = unittest.mock._importer(target) + + def decorator(func): + for attribute in attributes: + patcher = unittest.mock.patch.object(target, attribute, **kwargs) + func = patcher(func) + return func + return decorator class HashableMixin(discord.mixins.EqualityComparable): -- cgit v1.2.3 From 834bd543d1d301bb853e713560a7447dc75f1ab8 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 17:53:40 -0700 Subject: Test `is_maybe_token` returns False for missing parts In practice, this won't ever happen since the regex wouldn't match strings with missing parts. However, the function does check it so may as well test it. It's not necessarily bound to always use inputs from the regex either I suppose. --- tests/bot/cogs/test_token_remover.py | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index b67602eb9..9e1d96a37 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -164,6 +164,16 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = TOKEN_RE.findall(token) self.assertEquals(len(results), 0) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): + """False should be returned for tokens which do not have all 3 parts.""" + cog = TokenRemover(self.bot) + return_value = cog.is_maybe_token("x.y") + + self.assertFalse(return_value) + valid_user.assert_not_called() + valid_time.assert_not_called() + def test_ignores_messages_with_invalid_tokens(self): """Messages with values that are invalid tokens are ignored.""" for content in ('foo.bar.baz', 'x.y.'): -- cgit v1.2.3 From ab5d194b90a7e068c8ab7171939f471e252ee073 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:11:31 -0700 Subject: Test is_maybe_token --- tests/bot/cogs/test_token_remover.py | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 9e1d96a37..85bbbdf6b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -174,13 +174,30 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): valid_user.assert_not_called() valid_time.assert_not_called() - def test_ignores_messages_with_invalid_tokens(self): - """Messages with values that are invalid tokens are ignored.""" - for content in ('foo.bar.baz', 'x.y.'): - with self.subTest(content=content): - self.msg.content = content - coroutine = self.cog.is_maybe_token(self.msg) - self.assertIsNone(asyncio.run(coroutine)) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + def test_is_maybe_token(self, valid_user, valid_time): + """Should return True if the user ID and timestamp are valid or return False otherwise.""" + cog = TokenRemover(self.bot) + subtests = ( + (False, True, False), + (True, False, False), + (True, True, True), + ) + + for user_return, time_return, expected in subtests: + valid_user.reset_mock() + valid_time.reset_mock() + + with self.subTest(user_return=user_return, time_return=time_return, expected=expected): + valid_user.return_value = user_return + valid_time.return_value = time_return + + actual = cog.is_maybe_token("x.y.z") + self.assertIs(actual, expected) + + valid_user.assert_called_once_with("x") + if user_return: + valid_time.assert_called_once_with("y") def test_censors_valid_tokens(self): """Valid tokens are censored.""" -- cgit v1.2.3 From 4b6fde69a7e193382701dccf80a5471ea7ccea72 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:22:31 -0700 Subject: Test token regex matches valid tokens --- tests/bot/cogs/test_token_remover.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 85bbbdf6b..7310b4637 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -164,6 +164,27 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = TOKEN_RE.findall(token) self.assertEquals(len(results), 0) + def test_regex_valid_tokens(self): + """Messages that look like tokens should be matched.""" + # Don't worry, the token's been invalidated. + tokens = ( + "x1.y2.z_3", + "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8" + ) + + for token in tokens: + with self.subTest(token=token): + results = TOKEN_RE.findall(token) + self.assertIn(token, results) + + def test_regex_matches_multiple_valid(self): + """Should support multiple matches in the middle of a string.""" + tokens = ["x.y.z", "a.b.c"] + message = f"garbage {tokens[0]} hello {tokens[1]} world" + + results = TOKEN_RE.findall(message) + self.assertEquals(tokens, results) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): """False should be returned for tokens which do not have all 3 parts.""" -- cgit v1.2.3 From d8d8e144adfe4c2de15dbbf4346e2eec548a9f67 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 10 May 2020 18:28:06 -0700 Subject: Correct the return type annotation for the autospec decorator --- tests/helpers.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/helpers.py b/tests/helpers.py index dfbe539ec..3cd8a63c0 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -4,7 +4,7 @@ import collections import itertools import logging import unittest.mock -from typing import Iterable, Optional +from typing import Callable, Iterable, Optional import discord from discord.ext.commands import Context @@ -23,7 +23,7 @@ for logger in logging.Logger.manager.loggerDict.values(): logger.setLevel(logging.CRITICAL) -def autospec(target, *attributes: str, **kwargs) -> unittest.mock._patch: +def autospec(target, *attributes: str, **kwargs) -> Callable: """Patch multiple `attributes` of a `target` with autospecced mocks and `spec_set` as True.""" # Caller's kwargs should take priority and overwrite the defaults. kwargs = {'spec_set': True, 'autospec': True, **kwargs} -- cgit v1.2.3 From 5b9bf9aba686f570322cb9996dd35d3ab669a162 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:26:16 -0700 Subject: Avoid instantiating the cog when testing static/class methods --- tests/bot/cogs/test_token_remover.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 7310b4637..6a8247070 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -93,10 +93,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_ignores_bot_messages(self, token_re): """The token finder should ignore messages authored by bots.""" - cog = TokenRemover(self.bot) self.msg.author.bot = True - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) token_re.findall.assert_not_called() @@ -105,11 +104,10 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" - cog = TokenRemover(self.bot) token_re.findall.return_value = () self.msg.content = "foobar" - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) token_re.findall.assert_called_once_with(self.msg.content) @@ -124,12 +122,11 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): side_effects = [False] * len(matches) side_effects[true_index] = True - cog = TokenRemover(self.bot) self.msg.content = "foobar" token_re.findall.return_value = matches is_maybe_token.side_effect = side_effects - return_value = cog.find_token_in_message(self.msg) + return_value = TokenRemover.find_token_in_message(self.msg) self.assertEqual(return_value, matches[true_index]) token_re.findall.assert_called_once_with(self.msg.content) @@ -188,8 +185,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): """False should be returned for tokens which do not have all 3 parts.""" - cog = TokenRemover(self.bot) - return_value = cog.is_maybe_token("x.y") + return_value = TokenRemover.is_maybe_token("x.y") self.assertFalse(return_value) valid_user.assert_not_called() @@ -198,7 +194,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token(self, valid_user, valid_time): """Should return True if the user ID and timestamp are valid or return False otherwise.""" - cog = TokenRemover(self.bot) subtests = ( (False, True, False), (True, False, False), @@ -213,7 +208,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): valid_user.return_value = user_return valid_time.return_value = time_return - actual = cog.is_maybe_token("x.y.z") + actual = TokenRemover.is_maybe_token("x.y.z") self.assertIs(actual, expected) valid_user.assert_called_once_with("x") -- cgit v1.2.3 From 2127239840085ba523d411899e0b7a188530df07 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:33:05 -0700 Subject: Simplify token remover's message mock * Rely on default values for the author * Set the content to a non-empty string --- tests/bot/cogs/test_token_remover.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 6a8247070..5ca863926 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -26,14 +26,10 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.bot.get_cog.return_value.send_log_message = AsyncMock() self.cog = TokenRemover(bot=self.bot) - self.msg = MockMessage(id=555, content='') - self.msg.author.__str__ = MagicMock() - self.msg.author.__str__.return_value = 'lemon' - self.msg.author.bot = False - self.msg.author.avatar_url_as.return_value = 'picture-lemon.png' - self.msg.author.id = 42 - self.msg.author.mention = '@lemon' + self.msg = MockMessage(id=555, content="hello world") self.msg.channel.mention = "#lemonade-stand" + self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) + self.msg.author.avatar_url_as.return_value = "picture-lemon.png" def test_is_valid_user_id_is_true_for_numeric_content(self): """A string decoding to numeric characters is a valid user ID.""" @@ -105,7 +101,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" token_re.findall.return_value = () - self.msg.content = "foobar" return_value = TokenRemover.find_token_in_message(self.msg) @@ -122,7 +117,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): side_effects = [False] * len(matches) side_effects[true_index] = True - self.msg.content = "foobar" token_re.findall.return_value = matches is_maybe_token.side_effect = side_effects -- cgit v1.2.3 From e4790b330da1605573b5d23615bfe62b481e1e04 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:37:59 -0700 Subject: Test token remover's message deletion --- tests/bot/cogs/test_token_remover.py | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5ca863926..d65ce2ce5 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -209,6 +209,15 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): if user_return: valid_time.assert_called_once_with("y") + async def test_delete_message(self): + """The message should be deleted, and a message should be sent to the same channel.""" + await TokenRemover.delete_message(self.msg) + + self.msg.delete.assert_called_once_with() + self.msg.channel.send.assert_called_once_with( + DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + ) + def test_censors_valid_tokens(self): """Valid tokens are censored.""" cases = ( -- cgit v1.2.3 From 567a5f9242912d6a3340c088c0ae1a62977a141e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 10:46:02 -0700 Subject: Test TokenRemover.format_log_message --- tests/bot/cogs/test_token_remover.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index d65ce2ce5..f5412e692 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -218,6 +218,22 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) ) + @autospec("bot.cogs.token_remover", "LOG_MESSAGE") + async def test_format_log_message(self, log_message): + """Should correctly format the log message with info from the message and token.""" + log_message.format.return_value = "Howdy" + return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") + + self.assertEqual(return_value, log_message.format.return_value) + log_message.format.assert_called_once_with( + author=self.msg.author, + author_id=self.msg.author.id, + channel=self.msg.channel.mention, + user_id="MTIz", + timestamp="DN9R_A", + hmac="xxx", + ) + def test_censors_valid_tokens(self): """Valid tokens are censored.""" cases = ( -- cgit v1.2.3 From f47cbef0b47ef11b8c1fd63076105e4cb7d73601 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:29:28 -0700 Subject: Test TokenRemover.take_action * Remove `bot.get_cog` mocks in `setUp` * Mock the logger cause it's easier to assert logs * Remove subtests * Assert helper functions were called * Create an autospec for ModLog --- tests/bot/cogs/test_token_remover.py | 73 +++++++++++++++--------------------- 1 file changed, 30 insertions(+), 43 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index f5412e692..3546e7964 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,11 +1,10 @@ -import asyncio -import logging import unittest from unittest import mock -from unittest.mock import AsyncMock, MagicMock +from unittest.mock import MagicMock from discord import Colour +from bot.cogs.moderation import ModLog from bot.cogs.token_remover import ( DELETION_MESSAGE_TEMPLATE, TOKEN_RE, @@ -22,8 +21,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def setUp(self): """Adds the cog, a bot, and a message to the instance for usage in tests.""" self.bot = MockBot() - self.bot.get_cog.return_value = MagicMock() - self.bot.get_cog.return_value.send_log_message = AsyncMock() self.cog = TokenRemover(bot=self.bot) self.msg = MockMessage(id=555, content="hello world") @@ -234,46 +231,36 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): hmac="xxx", ) - def test_censors_valid_tokens(self): - """Valid tokens are censored.""" - cases = ( - # (content, censored_token) - ('MTIz.DN9R_A.xyz', 'MTIz.DN9R_A.xxx'), + @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) + @autospec("bot.cogs.token_remover", "log") + @autospec(TokenRemover, "delete_message", "format_log_message") + async def test_take_action(self, delete_message, format_log_message, logger, mod_log_property): + """Should delete the message and send a mod log.""" + cog = TokenRemover(self.bot) + mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) + token = "MTIz.DN9R_A.xyz" + log_msg = "testing123" + + mod_log_property.return_value = mod_log + format_log_message.return_value = log_msg + + await cog.take_action(self.msg, token) + + delete_message.assert_awaited_once_with(self.msg) + format_log_message.assert_called_once_with(self.msg, token) + logger.debug.assert_called_with(log_msg) + self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") + + mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) + mod_log.send_log_message.assert_called_once_with( + icon_url=Icons.token_removed, + colour=Colour(Colours.soft_red), + title="Token removed!", + text=log_msg, + thumbnail=self.msg.author.avatar_url_as.return_value, + channel_id=Channels.mod_alerts ) - for content, censored_token in cases: - with self.subTest(content=content, censored_token=censored_token): - self.msg.content = content - coroutine = self.cog.on_message(self.msg) - with self.assertLogs(logger='bot.cogs.token_remover', level=logging.DEBUG) as cm: - self.assertIsNone(asyncio.run(coroutine)) # no return value - - [line] = cm.output - log_message = ( - "Censored a seemingly valid token sent by " - "lemon (`42`) in #lemonade-stand, " - f"token was `{censored_token}`" - ) - self.assertIn(log_message, line) - - self.msg.delete.assert_called_once_with() - self.msg.channel.send.assert_called_once_with( - DELETION_MESSAGE_TEMPLATE.format(mention='@lemon') - ) - self.bot.get_cog.assert_called_with('ModLog') - self.msg.author.avatar_url_as.assert_called_once_with(static_format='png') - - mod_log = self.bot.get_cog.return_value - mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) - mod_log.send_log_message.assert_called_once_with( - icon_url=Icons.token_removed, - colour=Colour(Colours.soft_red), - title="Token removed!", - text=log_message, - thumbnail='picture-lemon.png', - channel_id=Channels.mod_alerts - ) - class TokenRemoverSetupTests(unittest.TestCase): """Tests setup of the `TokenRemover` cog.""" -- cgit v1.2.3 From 5734a4d84922a9497014dfeb3eba31ad3c57536f Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:44:08 -0700 Subject: Refactor `TokenRemoverSetupTests` and add a more thorough test The test now ensures the cog is instantiated and that the instance is passed as an argument to `add_cog`. --- tests/bot/cogs/test_token_remover.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 3546e7964..c377de7b2 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -262,11 +262,15 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): ) -class TokenRemoverSetupTests(unittest.TestCase): - """Tests setup of the `TokenRemover` cog.""" +class TokenRemoverExtensionTests(unittest.TestCase): + """Tests for the token_remover extension.""" - def test_setup(self): - """Setup of the extension should call add_cog.""" + @autospec("bot.cogs.token_remover", "TokenRemover") + def test_extension_setup(self, cog): + """The TokenRemover cog should be added.""" bot = MockBot() setup_cog(bot) + + cog.assert_called_once_with(bot) bot.add_cog.assert_called_once() + self.assertTrue(isinstance(bot.add_cog.call_args.args[0], TokenRemover)) -- cgit v1.2.3 From d0303d715d485842a2d5c906099d767d74cf8bd8 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:45:50 -0700 Subject: Replace deprecated assertion methods --- tests/bot/cogs/test_token_remover.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index c377de7b2..aecb51403 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -150,7 +150,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): results = TOKEN_RE.findall(token) - self.assertEquals(len(results), 0) + self.assertEqual(len(results), 0) def test_regex_valid_tokens(self): """Messages that look like tokens should be matched.""" @@ -171,7 +171,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): message = f"garbage {tokens[0]} hello {tokens[1]} world" results = TOKEN_RE.findall(message) - self.assertEquals(tokens, results) + self.assertEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- cgit v1.2.3 From 862153f2e4ab5b1408719fb2c1abc5143cfb15ce Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:47:40 -0700 Subject: Clean up token remover test imports --- tests/bot/cogs/test_token_remover.py | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index aecb51403..5cc8c7ad1 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -4,14 +4,10 @@ from unittest.mock import MagicMock from discord import Colour +from bot import constants +from bot.cogs import token_remover from bot.cogs.moderation import ModLog -from bot.cogs.token_remover import ( - DELETION_MESSAGE_TEMPLATE, - TOKEN_RE, - TokenRemover, - setup as setup_cog, -) -from bot.constants import Channels, Colours, Event, Icons +from bot.cogs.token_remover import TokenRemover from tests.helpers import MockBot, MockMessage, autospec @@ -149,7 +145,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = TOKEN_RE.findall(token) + results = token_remover.TOKEN_RE.findall(token) self.assertEqual(len(results), 0) def test_regex_valid_tokens(self): @@ -162,7 +158,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = TOKEN_RE.findall(token) + results = token_remover.TOKEN_RE.findall(token) self.assertIn(token, results) def test_regex_matches_multiple_valid(self): @@ -170,7 +166,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): tokens = ["x.y.z", "a.b.c"] message = f"garbage {tokens[0]} hello {tokens[1]} world" - results = TOKEN_RE.findall(message) + results = token_remover.TOKEN_RE.findall(message) self.assertEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") @@ -212,7 +208,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.delete.assert_called_once_with() self.msg.channel.send.assert_called_once_with( - DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) ) @autospec("bot.cogs.token_remover", "LOG_MESSAGE") @@ -251,14 +247,14 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): logger.debug.assert_called_with(log_msg) self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") - mod_log.ignore.assert_called_once_with(Event.message_delete, self.msg.id) + mod_log.ignore.assert_called_once_with(constants.Event.message_delete, self.msg.id) mod_log.send_log_message.assert_called_once_with( - icon_url=Icons.token_removed, - colour=Colour(Colours.soft_red), + icon_url=constants.Icons.token_removed, + colour=Colour(constants.Colours.soft_red), title="Token removed!", text=log_msg, thumbnail=self.msg.author.avatar_url_as.return_value, - channel_id=Channels.mod_alerts + channel_id=constants.Channels.mod_alerts ) @@ -269,7 +265,7 @@ class TokenRemoverExtensionTests(unittest.TestCase): def test_extension_setup(self, cog): """The TokenRemover cog should be added.""" bot = MockBot() - setup_cog(bot) + token_remover.setup(bot) cog.assert_called_once_with(bot) bot.add_cog.assert_called_once() -- cgit v1.2.3 From 4701b0da36c7f42792c0af258b785076237fd661 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 11 May 2020 11:56:15 -0700 Subject: Use subtests for valid ID/timestamp tests and test non-ASCII inputs --- tests/bot/cogs/test_token_remover.py | 43 +++++++++++++++++++++--------------- 1 file changed, 25 insertions(+), 18 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5cc8c7ad1..f1a56c235 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -24,24 +24,31 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) self.msg.author.avatar_url_as.return_value = "picture-lemon.png" - def test_is_valid_user_id_is_true_for_numeric_content(self): - """A string decoding to numeric characters is a valid user ID.""" - # MTIz = base64(123) - self.assertTrue(TokenRemover.is_valid_user_id('MTIz')) - - def test_is_valid_user_id_is_false_for_alphabetic_content(self): - """A string decoding to alphabetic characters is not a valid user ID.""" - # YWJj = base64(abc) - self.assertFalse(TokenRemover.is_valid_user_id('YWJj')) - - def test_is_valid_timestamp_is_true_for_valid_timestamps(self): - """A string decoding to a valid timestamp should be recognized as such.""" - self.assertTrue(TokenRemover.is_valid_timestamp('DN9r_A')) - - def test_is_valid_timestamp_is_false_for_invalid_values(self): - """A string not decoding to a valid timestamp should not be recognized as such.""" - # MTIz = base64(123) - self.assertFalse(TokenRemover.is_valid_timestamp('MTIz')) + def test_is_valid_user_id(self): + """Should correctly discern valid user IDs and ignore non-numeric and non-ASCII IDs.""" + subtests = ( + ("MTIz", True), # base64(123) + ("YWJj", False), # base64(abc) + ("λδµ", False), + ) + + for user_id, is_valid in subtests: + with self.subTest(user_id=user_id, is_valid=is_valid): + result = TokenRemover.is_valid_user_id(user_id) + self.assertIs(result, is_valid) + + def test_is_valid_timestamp(self): + """Should correctly discern valid timestamps.""" + subtests = ( + ("DN9r_A", True), + ("MTIz", False), # base64(123) + ("λδµ", False), + ) + + for timestamp, is_valid in subtests: + with self.subTest(timestamp=timestamp, is_valid=is_valid): + result = TokenRemover.is_valid_timestamp(timestamp) + self.assertIs(result, is_valid) def test_mod_log_property(self): """The `mod_log` property should ask the bot to return the `ModLog` cog.""" -- cgit v1.2.3 From 31aff51655d3783bc70f04628f189cf3c3591028 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 13 May 2020 18:58:43 -0700 Subject: Fix a test needlessly being a coroutine --- tests/bot/cogs/test_token_remover.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index f1a56c235..8e743a715 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -219,7 +219,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): ) @autospec("bot.cogs.token_remover", "LOG_MESSAGE") - async def test_format_log_message(self, log_message): + def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" log_message.format.return_value = "Howdy" return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") -- cgit v1.2.3 From 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 47886501fb7d030f1cb91c69413058e3ffcb76bf Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 20:47:32 -0700 Subject: Test token regex won't match non-base64 characters --- tests/bot/cogs/test_token_remover.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 8e743a715..dbea5ad1b 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -144,10 +144,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): "x..z", " . . ", "\n.\n.\n", - "'.'.'", - '"."."', - "(.(.(", - ").).)" + "hellö.world.bye", + "base64.nötbåse64.morebase64", + "19jd3J.dfkm3d.€víł§tüff", ) for token in tokens: -- cgit v1.2.3 From e76099d48b9a895c48e58c5f5489886f4191eeb6 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 20:50:30 -0700 Subject: Add more valid tokens to test the regex with --- tests/bot/cogs/test_token_remover.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index dbea5ad1b..6a280f358 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -156,10 +156,12 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_regex_valid_tokens(self): """Messages that look like tokens should be matched.""" - # Don't worry, the token's been invalidated. + # Don't worry, these tokens have been invalidated. tokens = ( - "x1.y2.z_3", - "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8" + "NDcyMjY1OTQzMDYy_DEzMz-y.XsyRkw.VXmErH7j511turNpfURmb0rVNm8", + "NDcyMjY1OTQzMDYyNDEzMzMy.Xrim9Q.Ysnu2wacjaKs7qnoo46S8Dm2us8", + "NDc1MDczNjI5Mzk5NTQ3OTA0.XsyR-w.sJf6omBPORBPju3WJEIAcwW9Zds", + "NDY3MjIzMjMwNjUwNzc3NjQx.XsySD_.s45jqDV_Iisn-symw0yDRrk_jf4", ) for token in tokens: -- cgit v1.2.3 From a8a216d0803b67a330ae092a17bea563f5012275 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:02:24 -0700 Subject: Fix valid token regex test It was broken due to the addition of groups. Rather than returning the full match, `findall` returns groups if any exist. The test was comparing a tuple of groups to the token string, which was of course failing. Now `fullmatch` is used cause it's simpler - just check for `None` and don't worry about iterating matches to search. --- tests/bot/cogs/test_token_remover.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 6a280f358..518bf91ca 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -166,8 +166,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): for token in tokens: with self.subTest(token=token): - results = token_remover.TOKEN_RE.findall(token) - self.assertIn(token, results) + results = token_remover.TOKEN_RE.fullmatch(token) + self.assertIsNotNone(results, f"{token} was not matched by the regex") def test_regex_matches_multiple_valid(self): """Should support multiple matches in the middle of a string.""" -- cgit v1.2.3 From 19cc849d4c70bc3e792460ad712aa308fa500462 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:07:21 -0700 Subject: Fix multiple match text for token regex It has to account for the addition of groups. It's easiest to compare the entire string so `finditer` is used to return re.Match objects; the tuples of `findall` would be cumbersome. Also threw in a change to use `assertCountEqual` cause the order doesn't really matter. --- tests/bot/cogs/test_token_remover.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 518bf91ca..2ecfae2bd 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -174,8 +174,9 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): tokens = ["x.y.z", "a.b.c"] message = f"garbage {tokens[0]} hello {tokens[1]} world" - results = token_remover.TOKEN_RE.findall(message) - self.assertEqual(tokens, results) + results = token_remover.TOKEN_RE.finditer(message) + results = [match[0] for match in results] + self.assertCountEqual(tokens, results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- cgit v1.2.3 From 300f8c093edea03855d94be179c64c328ec842ac Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 25 May 2020 21:09:04 -0700 Subject: Use real token values for testing multiple matches in regex --- tests/bot/cogs/test_token_remover.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 2ecfae2bd..971bc93fc 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -171,12 +171,13 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): def test_regex_matches_multiple_valid(self): """Should support multiple matches in the middle of a string.""" - tokens = ["x.y.z", "a.b.c"] - message = f"garbage {tokens[0]} hello {tokens[1]} world" + token_1 = "NDY3MjIzMjMwNjUwNzc3NjQx.XsyWGg.uFNEQPCc4ePwGh7egG8UicQssz8" + token_2 = "NDcyMjY1OTQzMDYyNDEzMzMy.XsyWMw.l8XPnDqb0lp-EiQ2g_0xVFT1pyc" + message = f"garbage {token_1} hello {token_2} world" results = token_remover.TOKEN_RE.finditer(message) results = [match[0] for match in results] - self.assertCountEqual(tokens, results) + self.assertCountEqual((token_1, token_2), results) @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): -- cgit v1.2.3 From 96db6087254c957fcb8fb45aad7ffcddb46ee839 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 17:08:18 -0700 Subject: Switch findall to finditer in assertions `find_token_in_message` now uses the latter so the tests should adjust accordingly. --- tests/bot/cogs/test_token_remover.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 971bc93fc..4fff3ab33 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -94,18 +94,18 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) - token_re.findall.assert_not_called() + token_re.finditer.assert_not_called() @autospec(TokenRemover, "is_maybe_token") @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): """None should be returned if the regex matches no tokens in a message.""" - token_re.findall.return_value = () + token_re.finditer.return_value = () return_value = TokenRemover.find_token_in_message(self.msg) self.assertIsNone(return_value) - token_re.findall.assert_called_once_with(self.msg.content) + token_re.finditer.assert_called_once_with(self.msg.content) is_maybe_token.assert_not_called() @autospec(TokenRemover, "is_maybe_token") @@ -123,7 +123,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): return_value = TokenRemover.find_token_in_message(self.msg) self.assertEqual(return_value, matches[true_index]) - token_re.findall.assert_called_once_with(self.msg.content) + token_re.finditer.assert_called_once_with(self.msg.content) # assert_has_calls isn't used cause it'd allow for extra calls before or after. # The function should short-circuit, so nothing past true_index should have been used. -- cgit v1.2.3 From f937032466a4124bacf217d1bfd0af097fc3395d Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 19:31:55 -0700 Subject: Adjust token remover tests to use the Token NamedTuple --- tests/bot/cogs/test_token_remover.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 4fff3ab33..65bc1ee58 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -7,7 +7,7 @@ from discord import Colour from bot import constants from bot.cogs import token_remover from bot.cogs.moderation import ModLog -from bot.cogs.token_remover import TokenRemover +from bot.cogs.token_remover import Token, TokenRemover from tests.helpers import MockBot, MockMessage, autospec @@ -224,17 +224,19 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @autospec("bot.cogs.token_remover", "LOG_MESSAGE") def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" + token = Token("NDY3MjIzMjMwNjUwNzc3NjQx", "XsySD_", "s45jqDV_Iisn-symw0yDRrk_jf4") log_message.format.return_value = "Howdy" - return_value = TokenRemover.format_log_message(self.msg, "MTIz.DN9R_A.xyz") + + return_value = TokenRemover.format_log_message(self.msg, token) self.assertEqual(return_value, log_message.format.return_value) log_message.format.assert_called_once_with( author=self.msg.author, author_id=self.msg.author.id, channel=self.msg.channel.mention, - user_id="MTIz", - timestamp="DN9R_A", - hmac="xxx", + user_id=token.user_id, + timestamp=token.timestamp, + hmac="x" * len(token.hmac), ) @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) @@ -244,7 +246,7 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): """Should delete the message and send a mod log.""" cog = TokenRemover(self.bot) mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) - token = "MTIz.DN9R_A.xyz" + token = mock.create_autospec(Token, spec_set=True, instance=True) log_msg = "testing123" mod_log_property.return_value = mod_log -- cgit v1.2.3 From 12b8f5002807144451a313180c639bf6b4925f2e Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Wed, 27 May 2020 20:00:33 -0700 Subject: Add more thorough and realistic inputs for token ID and timestamp tests The tests for valid inputs and invalid inputs were split to make them more readable. --- tests/bot/cogs/test_token_remover.py | 70 ++++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 18 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 65bc1ee58..ffe76865a 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -24,31 +24,65 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.msg.author.__str__ = MagicMock(return_value=self.msg.author.name) self.msg.author.avatar_url_as.return_value = "picture-lemon.png" - def test_is_valid_user_id(self): - """Should correctly discern valid user IDs and ignore non-numeric and non-ASCII IDs.""" - subtests = ( - ("MTIz", True), # base64(123) - ("YWJj", False), # base64(abc) - ("λδµ", False), + def test_is_valid_user_id_valid(self): + """Should consider user IDs valid if they decode entirely to ASCII digits.""" + ids = ( + "NDcyMjY1OTQzMDYyNDEzMzMy", + "NDc1MDczNjI5Mzk5NTQ3OTA0", + "NDY3MjIzMjMwNjUwNzc3NjQx", ) - for user_id, is_valid in subtests: - with self.subTest(user_id=user_id, is_valid=is_valid): + for user_id in ids: + with self.subTest(user_id=user_id): result = TokenRemover.is_valid_user_id(user_id) - self.assertIs(result, is_valid) + self.assertTrue(result) + + def test_is_valid_user_id_invalid(self): + """Should consider non-digit and non-ASCII IDs invalid.""" + ids = ( + ("SGVsbG8gd29ybGQ", "non-digit ASCII"), + ("0J_RgNC40LLQtdGCINC80LjRgA", "cyrillic text"), + ("4pO14p6L4p6C4pG34p264pGl8J-EiOKSj-KCieKBsA", "Unicode digits"), + ("4oaA4oaB4oWh4oWi4Lyz4Lyq4Lyr4LG9", "Unicode numerals"), + ("8J2fjvCdn5nwnZ-k8J2fr_Cdn7rgravvvJngr6c", "Unicode decimals"), + ("{hello}[world]&(bye!)", "ASCII invalid Base64"), + ("Þíß-ï§-ňøẗ-våłìÐ", "Unicode invalid Base64"), + ) - def test_is_valid_timestamp(self): - """Should correctly discern valid timestamps.""" - subtests = ( - ("DN9r_A", True), - ("MTIz", False), # base64(123) - ("λδµ", False), + for user_id, msg in ids: + with self.subTest(msg=msg): + result = TokenRemover.is_valid_user_id(user_id) + self.assertFalse(result) + + def test_is_valid_timestamp_valid(self): + """Should consider timestamps valid if they're greater than the Discord epoch.""" + timestamps = ( + "XsyRkw", + "Xrim9Q", + "XsyR-w", + "XsySD_", + "Dn9r_A", + ) + + for timestamp in timestamps: + with self.subTest(timestamp=timestamp): + result = TokenRemover.is_valid_timestamp(timestamp) + self.assertTrue(result) + + def test_is_valid_timestamp_invalid(self): + """Should consider timestamps invalid if they're before Discord epoch or can't be parsed.""" + timestamps = ( + ("B4Yffw", "DISCORD_EPOCH - TOKEN_EPOCH - 1"), + ("ew", "123"), + ("AoIKgA", "42076800"), + ("{hello}[world]&(bye!)", "ASCII invalid Base64"), + ("Þíß-ï§-ňøẗ-våłìÐ", "Unicode invalid Base64"), ) - for timestamp, is_valid in subtests: - with self.subTest(timestamp=timestamp, is_valid=is_valid): + for timestamp, msg in timestamps: + with self.subTest(msg=msg): result = TokenRemover.is_valid_timestamp(timestamp) - self.assertIs(result, is_valid) + self.assertFalse(result) def test_mod_log_property(self): """The `mod_log` property should ask the bot to return the `ModLog` cog.""" -- cgit v1.2.3 From 67472080fef5c38b21d74daa2178c3f35081b58f Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 19:52:41 -0700 Subject: Remove is_maybe_token tests The function was removed due to redundancy. Therefore, its tests are obsolete. --- tests/bot/cogs/test_token_remover.py | 33 --------------------------------- 1 file changed, 33 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index ffe76865a..5dd12636c 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -213,39 +213,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = [match[0] for match in results] self.assertCountEqual((token_1, token_2), results) - @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") - def test_is_maybe_token_missing_part_returns_false(self, valid_user, valid_time): - """False should be returned for tokens which do not have all 3 parts.""" - return_value = TokenRemover.is_maybe_token("x.y") - - self.assertFalse(return_value) - valid_user.assert_not_called() - valid_time.assert_not_called() - - @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") - def test_is_maybe_token(self, valid_user, valid_time): - """Should return True if the user ID and timestamp are valid or return False otherwise.""" - subtests = ( - (False, True, False), - (True, False, False), - (True, True, True), - ) - - for user_return, time_return, expected in subtests: - valid_user.reset_mock() - valid_time.reset_mock() - - with self.subTest(user_return=user_return, time_return=time_return, expected=expected): - valid_user.return_value = user_return - valid_time.return_value = time_return - - actual = TokenRemover.is_maybe_token("x.y.z") - self.assertIs(actual, expected) - - valid_user.assert_called_once_with("x") - if user_return: - valid_time.assert_called_once_with("y") - async def test_delete_message(self): """The message should be deleted, and a message should be sent to the same channel.""" await TokenRemover.delete_message(self.msg) -- cgit v1.2.3 From 84cd8235863acc80b7f140309424c33180cc34ea Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 20:32:48 -0700 Subject: Adjust find_token_in_message tests for the recent cog changes It now supports the changes that switched to finditer, added match groups, and added the Token NamedTuple. It also accounts for the is_maybe_token function being removed. For the sake of simplicity, call assertions on is_valid_user_id and is_valid_timestamp were not made. --- tests/bot/cogs/test_token_remover.py | 39 ++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 19 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 5dd12636c..8238e235a 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -1,4 +1,5 @@ import unittest +from re import Match from unittest import mock from unittest.mock import MagicMock @@ -130,9 +131,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.finditer.assert_not_called() - @autospec(TokenRemover, "is_maybe_token") @autospec("bot.cogs.token_remover", "TOKEN_RE") - def test_find_token_no_matches_returns_none(self, token_re, is_maybe_token): + def test_find_token_no_matches(self, token_re): """None should be returned if the regex matches no tokens in a message.""" token_re.finditer.return_value = () @@ -140,30 +140,31 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertIsNone(return_value) token_re.finditer.assert_called_once_with(self.msg.content) - is_maybe_token.assert_not_called() - @autospec(TokenRemover, "is_maybe_token") + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + @autospec("bot.cogs.token_remover", "Token") @autospec("bot.cogs.token_remover", "TOKEN_RE") - def test_find_token_returns_found_token(self, token_re, is_maybe_token): - """The found token should be returned.""" - true_index = 1 - matches = ("foo", "bar", "baz") - side_effects = [False] * len(matches) - side_effects[true_index] = True - - token_re.findall.return_value = matches - is_maybe_token.side_effect = side_effects + def test_find_token_valid_match(self, token_re, token_cls, is_valid_id, is_valid_timestamp): + """The first match with a valid user ID and timestamp should be returned as a `Token`.""" + matches = [ + mock.create_autospec(Match, spec_set=True, instance=True), + mock.create_autospec(Match, spec_set=True, instance=True), + ] + tokens = [ + mock.create_autospec(Token, spec_set=True, instance=True), + mock.create_autospec(Token, spec_set=True, instance=True), + ] + + token_re.finditer.return_value = matches + token_cls.side_effect = tokens + is_valid_id.side_effect = (False, True) # The 1st match will be invalid, 2nd one valid. + is_valid_timestamp.return_value = True return_value = TokenRemover.find_token_in_message(self.msg) - self.assertEqual(return_value, matches[true_index]) + self.assertEqual(tokens[1], return_value) token_re.finditer.assert_called_once_with(self.msg.content) - # assert_has_calls isn't used cause it'd allow for extra calls before or after. - # The function should short-circuit, so nothing past true_index should have been used. - calls = [mock.call(match) for match in matches[:true_index + 1]] - self.assertEqual(is_maybe_token.mock_calls, calls) - def test_regex_invalid_tokens(self): """Messages without anything looking like a token are not matched.""" tokens = ( -- cgit v1.2.3 From 5930a044b8347019d474a809fc86f89263574ad0 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Thu, 28 May 2020 20:33:34 -0700 Subject: Test find_token_in_message returns None for invalid matches This covers the case when a token is matched, but its user ID and timestamp turn out to be invalid. --- tests/bot/cogs/test_token_remover.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 8238e235a..9b4b04ecd 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -165,6 +165,21 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): self.assertEqual(tokens[1], return_value) token_re.finditer.assert_called_once_with(self.msg.content) + @autospec(TokenRemover, "is_valid_user_id", "is_valid_timestamp") + @autospec("bot.cogs.token_remover", "Token") + @autospec("bot.cogs.token_remover", "TOKEN_RE") + def test_find_token_invalid_matches(self, token_re, token_cls, is_valid_id, is_valid_timestamp): + """None should be returned if no matches have valid user IDs or timestamps.""" + token_re.finditer.return_value = [mock.create_autospec(Match, spec_set=True, instance=True)] + token_cls.return_value = mock.create_autospec(Token, spec_set=True, instance=True) + is_valid_id.return_value = False + is_valid_timestamp.return_value = False + + return_value = TokenRemover.find_token_in_message(self.msg) + + self.assertIsNone(return_value) + token_re.finditer.assert_called_once_with(self.msg.content) + def test_regex_invalid_tokens(self): """Messages without anything looking like a token are not matched.""" tokens = ( -- cgit v1.2.3 From 876b4846f612fe0011cc2e0b498b4df9e54d74cb Mon Sep 17 00:00:00 2001 From: Leon Sandøy Date: Sun, 31 May 2020 19:17:07 +0200 Subject: Add support for bool values in RedisCache We're gonna need this for the help channel handling, and it seems like a reasonable type to support anyway. It requires a tiny bit of special handling, but nothing outrageous. --- bot/utils/redis_cache.py | 14 ++++++++++++-- tests/bot/utils/test_redis_cache.py | 4 +++- 2 files changed, 15 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/bot/utils/redis_cache.py b/bot/utils/redis_cache.py index de80cee84..2926e7a89 100644 --- a/bot/utils/redis_cache.py +++ b/bot/utils/redis_cache.py @@ -2,6 +2,7 @@ from __future__ import annotations import asyncio import logging +from distutils.util import strtobool from functools import partialmethod from typing import Any, Dict, ItemsView, Optional, Tuple, Union @@ -11,7 +12,7 @@ log = logging.getLogger(__name__) # Type aliases RedisKeyType = Union[str, int] -RedisValueType = Union[str, int, float] +RedisValueType = Union[str, int, float, bool] RedisKeyOrValue = Union[RedisKeyType, RedisValueType] # Prefix tuples @@ -20,6 +21,7 @@ _VALUE_PREFIXES = ( ("f|", float), ("i|", int), ("s|", str), + ("b|", bool), ) _KEY_PREFIXES = ( ("i|", int), @@ -117,7 +119,8 @@ class RedisCache: def _to_typestring(key_or_value: RedisKeyOrValue, prefixes: _PrefixTuple) -> str: """Turn a valid Redis type into a typestring.""" for prefix, _type in prefixes: - if isinstance(key_or_value, _type): + # isinstance is a bad idea here, because isintance(False, int) == True. + if type(key_or_value) is _type: return f"{prefix}{key_or_value}" raise TypeError(f"RedisCache._to_typestring only supports the following: {prefixes}.") @@ -131,6 +134,13 @@ class RedisCache: # Now we convert our unicode string back into the type it originally was. for prefix, _type in prefixes: if key_or_value.startswith(prefix): + + # For booleans, we need special handling because bool("False") is True. + if prefix == "b|": + value = key_or_value[len(prefix):] + return bool(strtobool(value)) + + # Otherwise we can just convert normally. return _type(key_or_value[len(prefix):]) raise TypeError(f"RedisCache._from_typestring only supports the following: {prefixes}.") diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 8c1a40640..62c411681 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -59,7 +59,9 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): test_cases = ( ('favorite_fruit', 'melon'), ('favorite_number', 86), - ('favorite_fraction', 86.54) + ('favorite_fraction', 86.54), + ('favorite_boolean', False), + ('other_boolean', True), ) # Test that we can get and set different types. -- cgit v1.2.3 From ebbaa6274cfc278c772593b193356aa8bf066de4 Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Sun, 31 May 2020 14:17:20 -0700 Subject: Remove redis namespace collision test --- tests/bot/utils/test_redis_cache.py | 10 ---------- 1 file changed, 10 deletions(-) (limited to 'tests') diff --git a/tests/bot/utils/test_redis_cache.py b/tests/bot/utils/test_redis_cache.py index 8c1a40640..e5d6e4078 100644 --- a/tests/bot/utils/test_redis_cache.py +++ b/tests/bot/utils/test_redis_cache.py @@ -44,16 +44,6 @@ class RedisCacheTests(unittest.IsolatedAsyncioTestCase): with self.assertRaises(RuntimeError): await bad_cache.set("test", "me_up_deadman") - def test_namespace_collision(self): - """Test that we prevent colliding namespaces.""" - bob_cache_1 = RedisCache() - bob_cache_1._set_namespace("BobRoss") - self.assertEqual(bob_cache_1._namespace, "BobRoss") - - bob_cache_2 = RedisCache() - bob_cache_2._set_namespace("BobRoss") - self.assertEqual(bob_cache_2._namespace, "BobRoss_") - async def test_set_get_item(self): """Test that users can set and get items from the RedisDict.""" test_cases = ( -- cgit v1.2.3 From 9b3ab7df5ae1ecf95705f2fab7d99fdb36eb98ea Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Tue, 2 Jun 2020 19:22:49 -0700 Subject: Token remover: remove the `delete_message` function It's redundant; there's no benefit here in abstracting two lines of code into a function. --- bot/cogs/token_remover.py | 9 ++------- tests/bot/cogs/test_token_remover.py | 19 +++++++------------ 2 files changed, 9 insertions(+), 19 deletions(-) (limited to 'tests') diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 46329e207..d55e079e9 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -79,7 +79,8 @@ class TokenRemover(Cog): async def take_action(self, msg: Message, found_token: Token) -> None: """Remove the `msg` containing the `found_token` and send a mod log message.""" self.mod_log.ignore(Event.message_delete, msg.id) - await self.delete_message(msg) + await msg.delete() + await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) log_message = self.format_log_message(msg, found_token) log.debug(log_message) @@ -96,12 +97,6 @@ class TokenRemover(Cog): self.bot.stats.incr("tokens.removed_tokens") - @staticmethod - async def delete_message(msg: Message) -> None: - """Remove a `msg` containing a token and send an explanatory message in the same channel.""" - await msg.delete() - await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) - @staticmethod def format_log_message(msg: Message, token: Token) -> str: """Return the log message to send for `token` being censored in `msg`.""" diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 9b4b04ecd..a10124d2d 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -229,15 +229,6 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): results = [match[0] for match in results] self.assertCountEqual((token_1, token_2), results) - async def test_delete_message(self): - """The message should be deleted, and a message should be sent to the same channel.""" - await TokenRemover.delete_message(self.msg) - - self.msg.delete.assert_called_once_with() - self.msg.channel.send.assert_called_once_with( - token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) - ) - @autospec("bot.cogs.token_remover", "LOG_MESSAGE") def test_format_log_message(self, log_message): """Should correctly format the log message with info from the message and token.""" @@ -258,8 +249,8 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) @autospec("bot.cogs.token_remover", "log") - @autospec(TokenRemover, "delete_message", "format_log_message") - async def test_take_action(self, delete_message, format_log_message, logger, mod_log_property): + @autospec(TokenRemover, "format_log_message") + async def test_take_action(self, format_log_message, logger, mod_log_property): """Should delete the message and send a mod log.""" cog = TokenRemover(self.bot) mod_log = mock.create_autospec(ModLog, spec_set=True, instance=True) @@ -271,7 +262,11 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): await cog.take_action(self.msg, token) - delete_message.assert_awaited_once_with(self.msg) + self.msg.delete.assert_called_once_with() + self.msg.channel.send.assert_called_once_with( + token_remover.DELETION_MESSAGE_TEMPLATE.format(mention=self.msg.author.mention) + ) + format_log_message.assert_called_once_with(self.msg, token) logger.debug.assert_called_with(log_msg) self.bot.stats.incr.assert_called_once_with("tokens.removed_tokens") -- cgit v1.2.3 From 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 be4902cbd66c2f7223608ddbfee4aa4f0e1a011a Mon Sep 17 00:00:00 2001 From: ItsDrike Date: Sat, 6 Jun 2020 22:11:53 +0200 Subject: Test for channel not silenced message --- tests/bot/cogs/moderation/test_silence.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_silence.py b/tests/bot/cogs/moderation/test_silence.py index 3fd149f04..ab3d0742a 100644 --- a/tests/bot/cogs/moderation/test_silence.py +++ b/tests/bot/cogs/moderation/test_silence.py @@ -127,10 +127,20 @@ class SilenceTests(unittest.IsolatedAsyncioTestCase): self.ctx.reset_mock() async def test_unsilence_sent_correct_discord_message(self): - """Proper reply after a successful unsilence.""" - with mock.patch.object(self.cog, "_unsilence", return_value=True): - await self.cog.unsilence.callback(self.cog, self.ctx) - self.ctx.send.assert_called_once_with(f"{Emojis.check_mark} unsilenced current channel.") + """Check if proper message was sent when unsilencing channel.""" + test_cases = ( + (True, f"{Emojis.check_mark} unsilenced current channel."), + (False, f"{Emojis.cross_mark} current channel was not silenced.") + ) + for _unsilence_patch_return, result_message in test_cases: + with self.subTest( + starting_silenced_state=_unsilence_patch_return, + result_message=result_message + ): + with mock.patch.object(self.cog, "_unsilence", return_value=_unsilence_patch_return): + await self.cog.unsilence.callback(self.cog, self.ctx) + self.ctx.send.assert_called_once_with(result_message) + self.ctx.reset_mock() async def test_silence_private_for_false(self): """Permissions are not set and `False` is returned in an already silenced channel.""" -- cgit v1.2.3 From 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 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 c7373fa1143a2d2f2d784a59d40bcb40ee765bfb Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 15 Jun 2020 10:26:23 -0700 Subject: Token remover: ignore DMs It's a private channel so there's no risk of a token "leaking". Furthermore, messages cannot be deleted in DMs. --- bot/cogs/token_remover.py | 3 +++ tests/bot/cogs/test_token_remover.py | 10 ++++++++++ 2 files changed, 13 insertions(+) (limited to 'tests') diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index d55e079e9..493479df9 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -63,6 +63,9 @@ class TokenRemover(Cog): See: https://discordapp.com/developers/docs/reference#snowflakes """ + if not msg.guild: + return # Ignore DMs; can't delete messages in there anyway. + found_token = self.find_token_in_message(msg) if found_token: await self.take_action(msg, found_token) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index a10124d2d..22c31d7b1 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -121,6 +121,16 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): find_token_in_message.assert_called_once_with(self.msg) take_action.assert_not_awaited() + @autospec(TokenRemover, "find_token_in_message") + async def test_on_message_ignores_dms(self, find_token_in_message): + """Shouldn't parse a message if it is a DM.""" + cog = TokenRemover(self.bot) + self.msg.guild = None + + await cog.on_message(self.msg) + + find_token_in_message.assert_not_called() + @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_ignores_bot_messages(self, token_re): """The token finder should ignore messages authored by bots.""" -- cgit v1.2.3 From 2fa7429327e787a65803c16609da21463723bfeb Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 15 Jun 2020 10:38:46 -0700 Subject: Token remover: move bot check to on_message It just makes more sense to me to filter out messages at an earlier stage. --- bot/cogs/token_remover.py | 8 +++----- tests/bot/cogs/test_token_remover.py | 23 +++++++---------------- 2 files changed, 10 insertions(+), 21 deletions(-) (limited to 'tests') diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 493479df9..1f7517501 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -63,8 +63,9 @@ class TokenRemover(Cog): See: https://discordapp.com/developers/docs/reference#snowflakes """ - if not msg.guild: - return # Ignore DMs; can't delete messages in there anyway. + # Ignore DMs; can't delete messages in there anyway. + if not msg.guild or msg.author.bot: + return found_token = self.find_token_in_message(msg) if found_token: @@ -115,9 +116,6 @@ class TokenRemover(Cog): @classmethod def find_token_in_message(cls, msg: Message) -> t.Optional[Token]: """Return a seemingly valid token found in `msg` or `None` if no token is found.""" - if msg.author.bot: - return - # Use finditer rather than search to guard against method calls prematurely returning the # token check (e.g. `message.channel.send` also matches our token pattern) for match in TOKEN_RE.finditer(msg.content): diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 22c31d7b1..98ea9f823 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -122,24 +122,15 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): take_action.assert_not_awaited() @autospec(TokenRemover, "find_token_in_message") - async def test_on_message_ignores_dms(self, find_token_in_message): - """Shouldn't parse a message if it is a DM.""" + async def test_on_message_ignores_dms_bots(self, find_token_in_message): + """Shouldn't parse a message if it is a DM or authored by a bot.""" cog = TokenRemover(self.bot) - self.msg.guild = None + dm_msg = MockMessage(guild=None) + bot_msg = MockMessage(author=MagicMock(bot=True)) - await cog.on_message(self.msg) - - find_token_in_message.assert_not_called() - - @autospec("bot.cogs.token_remover", "TOKEN_RE") - def test_find_token_ignores_bot_messages(self, token_re): - """The token finder should ignore messages authored by bots.""" - self.msg.author.bot = True - - return_value = TokenRemover.find_token_in_message(self.msg) - - self.assertIsNone(return_value) - token_re.finditer.assert_not_called() + for msg in (dm_msg, bot_msg): + await cog.on_message(msg) + find_token_in_message.assert_not_called() @autospec("bot.cogs.token_remover", "TOKEN_RE") def test_find_token_no_matches(self, token_re): -- cgit v1.2.3 From 3aecf14419c87e533d47fe082abeb54ca9edb73c Mon Sep 17 00:00:00 2001 From: MarkKoz Date: Mon, 15 Jun 2020 10:49:18 -0700 Subject: Token remover: exit early if message already deleted --- bot/cogs/token_remover.py | 10 ++++++++-- tests/bot/cogs/test_token_remover.py | 15 ++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) (limited to 'tests') diff --git a/bot/cogs/token_remover.py b/bot/cogs/token_remover.py index 1f7517501..ef979f222 100644 --- a/bot/cogs/token_remover.py +++ b/bot/cogs/token_remover.py @@ -4,7 +4,7 @@ import logging import re import typing as t -from discord import Colour, Message +from discord import Colour, Message, NotFound from discord.ext.commands import Cog from bot import utils @@ -83,7 +83,13 @@ class TokenRemover(Cog): async def take_action(self, msg: Message, found_token: Token) -> None: """Remove the `msg` containing the `found_token` and send a mod log message.""" self.mod_log.ignore(Event.message_delete, msg.id) - await msg.delete() + + try: + await msg.delete() + except NotFound: + log.debug(f"Failed to remove token in message {msg.id}: message already deleted.") + return + await msg.channel.send(DELETION_MESSAGE_TEMPLATE.format(mention=msg.author.mention)) log_message = self.format_log_message(msg, found_token) diff --git a/tests/bot/cogs/test_token_remover.py b/tests/bot/cogs/test_token_remover.py index 98ea9f823..3349caa73 100644 --- a/tests/bot/cogs/test_token_remover.py +++ b/tests/bot/cogs/test_token_remover.py @@ -3,7 +3,7 @@ from re import Match from unittest import mock from unittest.mock import MagicMock -from discord import Colour +from discord import Colour, NotFound from bot import constants from bot.cogs import token_remover @@ -282,6 +282,19 @@ class TokenRemoverTests(unittest.IsolatedAsyncioTestCase): channel_id=constants.Channels.mod_alerts ) + @mock.patch.object(TokenRemover, "mod_log", new_callable=mock.PropertyMock) + async def test_take_action_delete_failure(self, mod_log_property): + """Shouldn't send any messages if the token message can't be deleted.""" + cog = TokenRemover(self.bot) + mod_log_property.return_value = mock.create_autospec(ModLog, spec_set=True, instance=True) + self.msg.delete.side_effect = NotFound(MagicMock(), MagicMock()) + + token = mock.create_autospec(Token, spec_set=True, instance=True) + await cog.take_action(self.msg, token) + + self.msg.delete.assert_called_once_with() + self.msg.channel.send.assert_not_awaited() + class TokenRemoverExtensionTests(unittest.TestCase): """Tests for the token_remover extension.""" -- cgit v1.2.3 From 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 581573f2ece96a9ec666795431ff21068e949a63 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 20 Jun 2020 01:20:35 +0200 Subject: Write unit test for `sub_clyde` --- tests/bot/utils/test_messages.py | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 tests/bot/utils/test_messages.py (limited to 'tests') diff --git a/tests/bot/utils/test_messages.py b/tests/bot/utils/test_messages.py new file mode 100644 index 000000000..9c22c9751 --- /dev/null +++ b/tests/bot/utils/test_messages.py @@ -0,0 +1,27 @@ +import unittest + +from bot.utils import messages + + +class TestMessages(unittest.TestCase): + """Tests for functions in the `bot.utils.messages` module.""" + + def test_sub_clyde(self): + """Uppercase E's and lowercase e's are substituted with their cyrillic counterparts.""" + sub_e = "\u0435" + sub_E = "\u0415" # noqa: N806: Uppercase E in variable name + + test_cases = ( + (None, None), + ("", ""), + ("clyde", f"clyd{sub_e}"), + ("CLYDE", f"CLYD{sub_E}"), + ("cLyDe", f"cLyD{sub_e}"), + ("BIGclyde", f"BIGclyd{sub_e}"), + ("small clydeus the unholy", f"small clyd{sub_e}us the unholy"), + ("BIGCLYDE, babyclyde", f"BIGCLYD{sub_E}, babyclyd{sub_e}"), + ) + + for username_in, username_out in test_cases: + with self.subTest(input=username_in, expected_output=username_out): + self.assertEqual(messages.sub_clyde(username_in), username_out) -- cgit v1.2.3 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 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