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 40e00ff17465fc5a5fe6b46487bfea37655cd7b9 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Thu, 18 Jun 2020 19:33:59 +0200 Subject: Incidents tests: write tests for `process_event` This also breaks the helpers import statement into a vertical list, as the amount of imports has grown too much. I still believe that this is a preferred alternative to accessing the helpers via module namespace, as we use them a lot, and the added visual noise would be annoying to read - their names are already descriptive enough. --- tests/bot/cogs/moderation/test_incidents.py | 102 +++++++++++++++++++++++++++- 1 file changed, 101 insertions(+), 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index c093afc8a..6158d5d20 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -1,3 +1,4 @@ +import asyncio import enum import logging import unittest @@ -7,7 +8,16 @@ import aiohttp import discord from bot.cogs.moderation import Incidents, incidents -from tests.helpers import MockAsyncWebhook, MockBot, MockMessage, MockReaction, MockTextChannel, MockUser +from tests.helpers import ( + MockAsyncWebhook, + MockBot, + MockMember, + MockMessage, + MockReaction, + MockRole, + MockTextChannel, + MockUser, +) class MockSignal(enum.Enum): @@ -250,6 +260,96 @@ class TestMakeConfirmationTask(TestIncidents): self.assertFalse(created_check(payload=MagicMock(message_id=0))) +@patch("bot.cogs.moderation.incidents.ALLOWED_ROLES", {1, 2}) +@patch("bot.cogs.moderation.incidents.Incidents.make_confirmation_task", AsyncMock()) # Generic awaitable +class TestProcessEvent(TestIncidents): + """Tests for the `Incidents.process_event` coroutine.""" + + @patch("bot.cogs.moderation.incidents.ALLOWED_ROLES", {1, 2}) + async def test_process_event_bad_role(self): + """The reaction is removed when the author lacks all allowed roles.""" + incident = MockMessage() + member = MockMember(roles=[MockRole(id=0)]) # Must have role 1 or 2 + + await self.cog_instance.process_event("reaction", incident, member) + incident.remove_reaction.assert_called_once_with("reaction", member) + + async def test_process_event_bad_emoji(self): + """ + The reaction is removed when an invalid emoji is used. + + This requires that we pass in a `member` with valid roles, as we need the role check + to succeed. + """ + incident = MockMessage() + member = MockMember(roles=[MockRole(id=1)]) # Member has allowed role + + await self.cog_instance.process_event("invalid_signal", incident, member) + incident.remove_reaction.assert_called_once_with("invalid_signal", member) + + async def test_process_event_no_archive_on_investigating(self): + """Message is not archived on `Signal.INVESTIGATING`.""" + with patch("bot.cogs.moderation.incidents.Incidents.archive", AsyncMock()) as mocked_archive: + await self.cog_instance.process_event( + reaction=incidents.Signal.INVESTIGATING.value, + incident=MockMessage(), + member=MockMember(roles=[MockRole(id=1)]), + ) + + mocked_archive.assert_not_called() + + async def test_process_event_no_delete_if_archive_fails(self): + """ + Original message is not deleted when `Incidents.archive` returns False. + + This is the way of signaling that the relay failed, and we should not remove the original, + as that would result in losing the incident record. + """ + incident = MockMessage() + + with patch("bot.cogs.moderation.incidents.Incidents.archive", AsyncMock(return_value=False)): + await self.cog_instance.process_event( + reaction=incidents.Signal.ACTIONED.value, + incident=incident, + member=MockMember(roles=[MockRole(id=1)]) + ) + + incident.delete.assert_not_called() + + async def test_process_event_confirmation_task_is_awaited(self): + """Task given by `Incidents.make_confirmation_task` is awaited before method exits.""" + mock_task = AsyncMock() + + with patch("bot.cogs.moderation.incidents.Incidents.make_confirmation_task", mock_task): + await self.cog_instance.process_event( + reaction=incidents.Signal.ACTIONED.value, + incident=MockMessage(), + member=MockMember(roles=[MockRole(id=1)]) + ) + + mock_task.assert_awaited() + + async def test_process_event_confirmation_task_timeout_is_handled(self): + """ + Confirmation task `asyncio.TimeoutError` is handled gracefully. + + We have `make_confirmation_task` return a mock with a side effect, and then catch the + exception should it propagate out of `process_event`. This is so that we can then manually + fail the test with a more informative message than just the plain traceback. + """ + mock_task = AsyncMock(side_effect=asyncio.TimeoutError()) + + try: + with patch("bot.cogs.moderation.incidents.Incidents.make_confirmation_task", mock_task): + await self.cog_instance.process_event( + reaction=incidents.Signal.ACTIONED.value, + incident=MockMessage(), + member=MockMember(roles=[MockRole(id=1)]) + ) + except asyncio.TimeoutError: + self.fail("TimeoutError was not handled gracefully, and propagated out of `process_event`!") + + class TestResolveMessage(TestIncidents): """Tests for the `Incidents.resolve_message` coroutine.""" -- cgit v1.2.3 From ed4097629601704f0c65fc40cceb5fd6757d4779 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 19 Jun 2020 14:32:31 +0200 Subject: Incidents tests: add helper for mocking async for-loops See the docstring. This does not make the ambition to be powerful enough to be included in `tests.helpers`, and is only intended for local purposes. --- tests/bot/cogs/moderation/test_incidents.py | 37 +++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 6158d5d20..7fa8847ef 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -1,6 +1,7 @@ import asyncio import enum import logging +import typing as t import unittest from unittest.mock import AsyncMock, MagicMock, call, patch @@ -20,6 +21,42 @@ from tests.helpers import ( ) +class MockAsyncIterable: + """ + Helper for mocking asynchronous for loops. + + It does not appear that the `unittest` library currently provides anything that would + allow us to simply mock an async iterator, such as `discord.TextChannel.history`. + + We therefore write our own helper to wrap a regular synchronous iterable, and feed + its values via `__anext__` rather than `__next__`. + + This class was written for the purposes of testing the `Incidents` cog - it may not + be generic enough to be placed in the `tests.helpers` module. + """ + + def __init__(self, messages: t.Iterable): + """Take a sync iterable to be wrapped.""" + self.iter_messages = iter(messages) + + def __aiter__(self): + """Return `self` as we provide the `__anext__` method.""" + return self + + async def __anext__(self): + """ + Feed the next item, or raise `StopAsyncIteration`. + + Since we're wrapping a sync iterator, it will communicate that it has been depleted + by raising a `StopIteration`. The `async for` construct does not expect it, and we + therefore need to substitute it for the appropriate exception type. + """ + try: + return next(self.iter_messages) + except StopIteration: + raise StopAsyncIteration + + class MockSignal(enum.Enum): A = "A" B = "B" -- cgit v1.2.3 From d93ed5d801c08b7fb084427906e7ac484ac3563f Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 19 Jun 2020 14:37:44 +0200 Subject: Incidents tests: write tests for `crawl_incidents` --- tests/bot/cogs/moderation/test_incidents.py | 58 +++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 7fa8847ef..4e6dfd5f7 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -209,6 +209,64 @@ class TestIncidents(unittest.IsolatedAsyncioTestCase): self.cog_instance = Incidents(MockBot()) +@patch("asyncio.sleep", AsyncMock()) # Prevent the coro from sleeping to speed up the test +class TestCrawlIncidents(TestIncidents): + """ + Tests for the `Incidents.crawl_incidents` coroutine. + + Apart from `test_crawl_incidents_waits_until_cache_ready`, all tests in this class + will patch the return values of `is_incident` and `has_signal` and then observe + whether the `AsyncMock` for `add_signals` was awaited or not. + + The `add_signals` mock is added by each test separately to ensure it is clean (has not + been awaited by another test yet). The mock can be reset, but this appears to be the + cleaner way. + + For each test, we inject a mock channel with a history of 1 message only (see: `setUp`). + """ + + def setUp(self): + """For each test, ensure `bot.get_channel` returns a channel with 1 arbitrary message.""" + super().setUp() # First ensure we get `cog_instance` from parent + + incidents_history = MagicMock(return_value=MockAsyncIterable([MockMessage()])) + self.cog_instance.bot.get_channel = MagicMock(return_value=MockTextChannel(history=incidents_history)) + + async def test_crawl_incidents_waits_until_cache_ready(self): + """ + The coroutine will await the `wait_until_guild_available` event. + + Since this task is schedule in the `__init__`, it is critical that it waits for the + cache to be ready, so that it can safely get the #incidents channel. + """ + await self.cog_instance.crawl_incidents() + self.cog_instance.bot.wait_until_guild_available.assert_awaited() + + @patch("bot.cogs.moderation.incidents.add_signals", AsyncMock()) + @patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=False)) # Message doesn't qualify + @patch("bot.cogs.moderation.incidents.has_signals", MagicMock(return_value=False)) + async def test_crawl_incidents_noop_if_is_not_incident(self): + """Signals are not added for a non-incident message.""" + await self.cog_instance.crawl_incidents() + incidents.add_signals.assert_not_awaited() + + @patch("bot.cogs.moderation.incidents.add_signals", AsyncMock()) + @patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=True)) # Message qualifies + @patch("bot.cogs.moderation.incidents.has_signals", MagicMock(return_value=True)) # But already has signals + async def test_crawl_incidents_noop_if_message_already_has_signals(self): + """Signals are not added for messages which already have them.""" + await self.cog_instance.crawl_incidents() + incidents.add_signals.assert_not_awaited() + + @patch("bot.cogs.moderation.incidents.add_signals", AsyncMock()) + @patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=True)) # Message qualifies + @patch("bot.cogs.moderation.incidents.has_signals", MagicMock(return_value=False)) # And doesn't have signals + async def test_crawl_incidents_add_signals_called(self): + """Message has signals added as it does not have them yet and qualifies as an incident.""" + await self.cog_instance.crawl_incidents() + incidents.add_signals.assert_awaited_once() + + class TestArchive(TestIncidents): """Tests for the `Incidents.archive` coroutine.""" -- cgit v1.2.3 From 9a58b45cad51c961ad34fa9de9aaa060446c54fd Mon Sep 17 00:00:00 2001 From: kwzrd Date: Fri, 19 Jun 2020 16:57:15 +0200 Subject: Incidents tests: write tests for `on_raw_reaction_add` --- tests/bot/cogs/moderation/test_incidents.py | 128 ++++++++++++++++++++++++++++ 1 file changed, 128 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 4e6dfd5f7..55b15ec9e 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -520,6 +520,134 @@ class TestResolveMessage(TestIncidents): self.assertIsNone(await self.cog_instance.resolve_message(123)) +@patch("bot.constants.Channels.incidents", 123) +class TestOnRawReactionAdd(TestIncidents): + """ + Tests for the `Incidents.on_raw_reaction_add` listener. + + Writing tests for this listener comes with additional complexity due to the listener + awaiting the `crawl_task` task. See `asyncSetUp` for further details, which attempts + to make unit testing this function possible. + """ + + def setUp(self): + """ + Prepare & assign `payload` attribute. + + This attribute represents an *ideal* payload which will not be rejected by the + listener. As each test will receive a fresh instance, it can be mutated to + observe how the listener's behaviour changes with different attributes on + the passed payload. + """ + super().setUp() # Ensure `cog_instance` is assigned + + self.payload = MagicMock( + discord.RawReactionActionEvent, + channel_id=123, # Patched at class level + message_id=456, + member=MockMember(bot=False), + emoji="reaction", + ) + + async def asyncSetUp(self): # noqa: N802 + """ + Prepare an empty task and assign it as `crawl_task`. + + It appears that the `unittest` framework does not provide anything for mocking + asyncio tasks. An `AsyncMock` instance can be called and then awaited, however, + it does not provide the `done` method or any other parts of the `asyncio.Task` + interface. + + Although we do not need to make any assertions about the task itself while + testing the listener, the code will still await it and call the `done` method, + and so we must inject something that will not fail on either action. + + Note that this is done in an `asyncSetUp`, which runs after `setUp`. + The justification is that creating an actual task requires the event + loop to be ready, which is not the case in the `setUp`. + """ + mock_task = asyncio.create_task(AsyncMock()()) # Mock async func, then a coro + self.cog_instance.crawl_task = mock_task + + async def test_on_raw_reaction_add_wrong_channel(self): + """ + Events outside of #incidents will be ignored. + + We check this by asserting that `resolve_message` was never queried. + """ + self.payload.channel_id = 0 + self.cog_instance.resolve_message = AsyncMock() + + await self.cog_instance.on_raw_reaction_add(self.payload) + self.cog_instance.resolve_message.assert_not_called() + + async def test_on_raw_reaction_add_user_is_bot(self): + """ + Events dispatched by bot accounts will be ignored. + + We check this by asserting that `resolve_message` was never queried. + """ + self.payload.member = MockMember(bot=True) + self.cog_instance.resolve_message = AsyncMock() + + await self.cog_instance.on_raw_reaction_add(self.payload) + self.cog_instance.resolve_message.assert_not_called() + + async def test_on_raw_reaction_add_message_doesnt_exist(self): + """ + Listener gracefully handles the case where `resolve_message` gives None. + + We check this by asserting that `process_event` was never called. + """ + self.cog_instance.process_event = AsyncMock() + self.cog_instance.resolve_message = AsyncMock(return_value=None) + + await self.cog_instance.on_raw_reaction_add(self.payload) + self.cog_instance.process_event.assert_not_called() + + async def test_on_raw_reaction_add_message_is_not_an_incident(self): + """ + The event won't be processed if the related message is not an incident. + + This is an edge-case that can happen if someone manually leaves a reaction + on a pinned message, or a comment. + + We check this by asserting that `process_event` was never called. + """ + self.cog_instance.process_event = AsyncMock() + self.cog_instance.resolve_message = AsyncMock(return_value=MockMessage()) + + with patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=False)): + await self.cog_instance.on_raw_reaction_add(self.payload) + + self.cog_instance.process_event.assert_not_called() + + async def test_on_raw_reaction_add_valid_event_is_processed(self): + """ + If the reaction event is valid, it is passed to `process_event`. + + This is the case when everything goes right: + * The reaction was placed in #incidents, and not by a bot + * The message was found successfully + * The message qualifies as an incident + + Additionally, we check that all arguments were passed as expected. + """ + incident = MockMessage(id=1) + + self.cog_instance.process_event = AsyncMock() + self.cog_instance.resolve_message = AsyncMock(return_value=incident) + + with patch("bot.cogs.moderation.incidents.is_incident", MagicMock(return_value=True)): + await self.cog_instance.on_raw_reaction_add(self.payload) + + self.cog_instance.process_event.assert_called_with( + "reaction", # Defined in `self.payload` + incident, + self.payload.member, + ) + + class TestOnMessage(TestIncidents): """ Tests for the `Incidents.on_message` listener. -- cgit v1.2.3 From 98b8947ab7865e33f18da8e2a62b26405676e8e4 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 20 Jun 2020 13:13:45 +0200 Subject: Incidents: try-except Signal creation Suggested by Mark during review. This follows the "ask for forgiveness rather than permission" paradigm, ends up being less code to read, and may be seen as more logical / safer. The `ALLOWED_EMOJI` set was renamed to `ALL_SIGNALS` as this now better communicates the set's purpose. Tests adjusted as appropriate. Co-authored-by: MarkKoz --- bot/cogs/moderation/incidents.py | 18 +++++++++++------- tests/bot/cogs/moderation/test_incidents.py | 8 ++++---- 2 files changed, 15 insertions(+), 11 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 089a5bc9f..41a98bcb7 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -33,9 +33,11 @@ class Signal(Enum): INVESTIGATING = Emojis.incident_investigating -# Reactions from roles not listed here, or using emoji not listed here, will be removed +# Reactions from roles not listed here will be removed ALLOWED_ROLES: t.Set[int] = {Roles.moderators, Roles.admins, Roles.owners} -ALLOWED_EMOJI: t.Set[str] = {signal.value for signal in Signal} + +# Message must have all of these emoji to pass the `has_signals` check +ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal} def is_incident(message: discord.Message) -> bool: @@ -56,7 +58,7 @@ def own_reactions(message: discord.Message) -> t.Set[str]: def has_signals(message: discord.Message) -> bool: """True if `message` already has all `Signal` reactions, False otherwise.""" - return ALLOWED_EMOJI.issubset(own_reactions(message)) + return ALL_SIGNALS.issubset(own_reactions(message)) async def add_signals(incident: discord.Message) -> None: @@ -96,7 +98,9 @@ class Incidents(Cog): * See: `on_message` On reaction: - * Remove reaction if not permitted (`ALLOWED_EMOJI`, `ALLOWED_ROLES`) + * Remove reaction if not permitted + * User does not have any of the roles in `ALLOWED_ROLES` + * Used emoji is not a `Signal` member * If `Signal.ACTIONED` or `Signal.NOT_ACTIONED` were chosen, attempt to relay the incident message to #incidents-archive * If relay successful, delete original message @@ -217,13 +221,13 @@ class Incidents(Cog): await incident.remove_reaction(reaction, member) return - if reaction not in ALLOWED_EMOJI: + try: + signal = Signal(reaction) + except ValueError: log.debug(f"Removing invalid reaction: emoji {reaction} is not a valid signal") await incident.remove_reaction(reaction, member) return - # If we reach this point, we know that `emoji` is a `Signal` member - signal = Signal(reaction) log.trace(f"Received signal: {signal}") if signal not in (Signal.ACTIONED, Signal.NOT_ACTIONED): diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 55b15ec9e..862736785 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -131,17 +131,17 @@ class TestOwnReactions(unittest.TestCase): self.assertSetEqual(incidents.own_reactions(message), {"A", "B"}) -@patch("bot.cogs.moderation.incidents.ALLOWED_EMOJI", {"A", "B"}) +@patch("bot.cogs.moderation.incidents.ALL_SIGNALS", {"A", "B"}) class TestHasSignals(unittest.TestCase): """ Assertions for the `has_signals` function. - We patch `ALLOWED_EMOJI` globally. Each test function then patches `own_reactions` + We patch `ALL_SIGNALS` globally. Each test function then patches `own_reactions` as appropriate. """ def test_has_signals_true(self): - """True when `own_reactions` returns all emoji in `ALLOWED_EMOJI`.""" + """True when `own_reactions` returns all emoji in `ALL_SIGNALS`.""" message = MockMessage() own_reactions = MagicMock(return_value={"A", "B"}) @@ -149,7 +149,7 @@ class TestHasSignals(unittest.TestCase): self.assertTrue(incidents.has_signals(message)) def test_has_signals_false(self): - """False when `own_reactions` does not return all emoji in `ALLOWED_EMOJI`.""" + """False when `own_reactions` does not return all emoji in `ALL_SIGNALS`.""" message = MockMessage() own_reactions = MagicMock(return_value={"A", "C"}) -- cgit v1.2.3 From 20dbd177f227511b9c3cb678ab45a67558cd3d7f Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 20 Jun 2020 13:15:43 +0200 Subject: Incidents tests: remove unnecessary patch This is already being patched at class-level. --- tests/bot/cogs/moderation/test_incidents.py | 1 - 1 file changed, 1 deletion(-) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 862736785..9f0553216 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -360,7 +360,6 @@ class TestMakeConfirmationTask(TestIncidents): class TestProcessEvent(TestIncidents): """Tests for the `Incidents.process_event` coroutine.""" - @patch("bot.cogs.moderation.incidents.ALLOWED_ROLES", {1, 2}) async def test_process_event_bad_role(self): """The reaction is removed when the author lacks all allowed roles.""" incident = MockMessage() -- cgit v1.2.3 From f240a970c6b97d201959d25a79a8babafed1c2b1 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sat, 20 Jun 2020 14:15:34 +0200 Subject: Incidents tests: assert webhook username is de-clyded See: a8b4e394d9da57287cd9497cd9bb0a97fa467e84 --- tests/bot/cogs/moderation/test_incidents.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) (limited to 'tests') diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 9f0553216..2fc9180cf 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -319,6 +319,25 @@ class TestArchive(TestIncidents): # Finally check that the method returned True self.assertTrue(archive_return) + async def test_archive_clyde_username(self): + """ + The archive webhook username is cleansed using `sub_clyde`. + + Discord will reject any webhook with "clyde" in the username field, as it impersonates + the official Clyde bot. Since we do not control what the username will be (the incident + author name is used), we must ensure the name is cleansed, otherwise the relay may fail. + + This test assumes the username is passed as a kwarg. If this test fails, please review + whether the passed argument is being retrieved correctly. + """ + webhook = MockAsyncWebhook() + self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) + + message_from_clyde = MockMessage(author=MockUser(name="clyde the great")) + await self.cog_instance.archive(message_from_clyde, MagicMock(incidents.Signal)) + + self.assertNotIn("clyde", webhook.send.call_args.kwargs["username"]) + class TestMakeConfirmationTask(TestIncidents): """ -- cgit v1.2.3 From 6fa8caed037b247a7c194f58a4635de7dae21fd2 Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sun, 21 Jun 2020 13:51:17 +0200 Subject: Incidents: implement `make_username` helper The justification is to incorporate the `actioned_by` name into the username in some way, and so the logical thing to do is to abstract this process into a helper so that it can easily be adjusted in the future. For now, I've chosen to separate the names by a pipe. Discord webhook username cannot exceed 80 characters in length, and so we cap it at this length by default. This is seen as more of an edge-case, but it should be accounted for, as we're not joining two names. The `max_length` param is configurable primarily for testing purposes, it probably should never be passed explicitly. This commit also provides two tests for the function. --- bot/cogs/moderation/incidents.py | 24 ++++++++++++++++++++++++ tests/bot/cogs/moderation/test_incidents.py | 29 +++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 040f2c0c8..2cce9b6fe 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -41,6 +41,30 @@ ALLOWED_ROLES: t.Set[int] = {Roles.moderators, Roles.admins, Roles.owners} ALL_SIGNALS: t.Set[str] = {signal.value for signal in Signal} +def make_username(reported_by: discord.Member, actioned_by: discord.Member, max_length: int = 80) -> str: + """ + Create a webhook-friendly username from the names of `reported_by` and `actioned_by`. + + If the resulting username length exceeds `max_length`, it will be capped at `max_length - 3` + and have 3 dots appended to the end. The default value is 80, which corresponds to the limit + Discord imposes on webhook username length. + + If the value of `max_length` is < 3, ValueError is raised. + """ + if max_length < 3: + raise ValueError(f"Maximum length cannot be less than 3: {max_length=}") + + username = f"{reported_by.name} | {actioned_by.name}" + log.trace(f"Generated webhook username: {username} (length: {len(username)})") + + if len(username) > max_length: + stop = max_length - 3 + username = f"{username[:stop]}..." + log.trace(f"Username capped at {max_length=}: {username}") + + return username + + def is_incident(message: discord.Message) -> bool: """True if `message` qualifies as an incident, False otherwise.""" conditions = ( diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 2fc9180cf..5700a5a35 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -68,6 +68,35 @@ mock_404 = discord.NotFound( ) +class TestMakeUsername(unittest.TestCase): + """Collection of tests for the `make_username` helper function.""" + + def test_make_username_raises(self): + """Raises `ValueError` on `max_length` < 3.""" + with self.assertRaises(ValueError): + incidents.make_username(MockMember(), MockMember(), max_length=2) + + def test_make_username_never_exceed_limit(self): + """ + The return string length is always less than or equal to `max_length`. + + For this test we pass `max_length=10` for convenience. The name of the first + user (`reported_by`) is always 1 character in length, but we generate names + for the `actioned_by` user starting at length 1 and up to length 20. + + Finally, we assert that the output length never exceeded 10 in total. + """ + user_a = MockMember(name="A") + + max_length = 10 + test_cases = (MockMember(name="B" * n) for n in range(1, 20)) + + for user_b in test_cases: + with self.subTest(user_a=user_a, user_b=user_b, max_length=max_length): + generated_username = incidents.make_username(user_a, user_b, max_length) + self.assertLessEqual(len(generated_username), max_length) + + @patch("bot.constants.Channels.incidents", 123) class TestIsIncident(unittest.TestCase): """ -- cgit v1.2.3 From a8d179d9b04f54b20c5e870bcfa85c78c42c8dca Mon Sep 17 00:00:00 2001 From: kwzrd Date: Sun, 21 Jun 2020 14:21:18 +0200 Subject: Incidents: append `actioned_by` to webhook username Incident author and the moderator who actioned report are now passed through `make_username` to create the webhook username. Tests adjusted as appropriate. --- bot/cogs/moderation/incidents.py | 9 +++++---- tests/bot/cogs/moderation/test_incidents.py | 23 +++++++++++++++++------ 2 files changed, 22 insertions(+), 10 deletions(-) (limited to 'tests') diff --git a/bot/cogs/moderation/incidents.py b/bot/cogs/moderation/incidents.py index 2cce9b6fe..72cc4b26c 100644 --- a/bot/cogs/moderation/incidents.py +++ b/bot/cogs/moderation/incidents.py @@ -172,13 +172,14 @@ class Incidents(Cog): log.debug("Crawl task finished!") - async def archive(self, incident: discord.Message, outcome: Signal) -> bool: + async def archive(self, incident: discord.Message, outcome: Signal, actioned_by: discord.Member) -> bool: """ Relay `incident` to the #incidents-archive channel. The following pieces of information are relayed: * Incident message content (clean, pingless) - * Incident author name (as webhook author) + * Incident author name (as webhook username) + * Name of user who actioned the incident (appended to webhook username) * Incident author avatar (as webhook avatar) * Resolution signal (`outcome`) @@ -194,7 +195,7 @@ class Incidents(Cog): # Now relay the incident message: discord.Message = await webhook.send( content=incident.clean_content, # Clean content will prevent mentions from pinging - username=sub_clyde(incident.author.name), + username=sub_clyde(make_username(incident.author, actioned_by)), avatar_url=incident.author.avatar_url, wait=True, # This makes the method return the sent Message object ) @@ -259,7 +260,7 @@ class Incidents(Cog): log.debug("Reaction was valid, but no action is currently defined for it") return - relay_successful = await self.archive(incident, signal) + relay_successful = await self.archive(incident, signal, actioned_by=member) if not relay_successful: log.trace("Original message will not be deleted as we failed to relay it to the archive") return diff --git a/tests/bot/cogs/moderation/test_incidents.py b/tests/bot/cogs/moderation/test_incidents.py index 5700a5a35..a811868e5 100644 --- a/tests/bot/cogs/moderation/test_incidents.py +++ b/tests/bot/cogs/moderation/test_incidents.py @@ -307,7 +307,9 @@ class TestArchive(TestIncidents): propagate out of the method, which is just as important. """ self.cog_instance.bot.fetch_webhook = AsyncMock(side_effect=mock_404) - self.assertFalse(await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock())) + + result = await self.cog_instance.archive(incident=MockMessage(), outcome=MagicMock(), actioned_by=MockMember()) + self.assertFalse(result) async def test_archive_relays_incident(self): """ @@ -332,12 +334,18 @@ class TestArchive(TestIncidents): author=MockUser(name="author_name", avatar_url="author_avatar"), id=123, ) - archive_return = await self.cog_instance.archive(incident, outcome=MagicMock(value="A")) + + with patch("bot.cogs.moderation.incidents.make_username", MagicMock(return_value="generated_username")): + archive_return = await self.cog_instance.archive( + incident=incident, + outcome=MagicMock(value="A"), + actioned_by=MockMember(name="moderator"), + ) # Check that the webhook was dispatched correctly webhook.send.assert_called_once_with( content="pingless message", - username="author_name", + username="generated_username", avatar_url="author_avatar", wait=True, ) @@ -354,7 +362,8 @@ class TestArchive(TestIncidents): Discord will reject any webhook with "clyde" in the username field, as it impersonates the official Clyde bot. Since we do not control what the username will be (the incident - author name is used), we must ensure the name is cleansed, otherwise the relay may fail. + author name, and actioning moderator names are used), we must ensure the name is cleansed, + otherwise the relay may fail. This test assumes the username is passed as a kwarg. If this test fails, please review whether the passed argument is being retrieved correctly. @@ -362,9 +371,11 @@ class TestArchive(TestIncidents): webhook = MockAsyncWebhook() self.cog_instance.bot.fetch_webhook = AsyncMock(return_value=webhook) - message_from_clyde = MockMessage(author=MockUser(name="clyde the great")) - await self.cog_instance.archive(message_from_clyde, MagicMock(incidents.Signal)) + # The `make_username` helper will return a string with "clyde" in it + with patch("bot.cogs.moderation.incidents.make_username", MagicMock(return_value="clyde the great")): + await self.cog_instance.archive(MockMessage(), MagicMock(incidents.Signal), MockMember()) + # Assert that the "clyde" was never passed to `send` self.assertNotIn("clyde", webhook.send.call_args.kwargs["username"]) -- cgit v1.2.3 From 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 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 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 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