Merge branch 'master' into hush-cog

7 files changed, 131 insertions, 58 deletions

diff --git a/tests/README.md b/tests/README.md
index be78821bf..4f62edd68 100644
--- a/tests/README.md
+++ b/tests/README.md
@@ -83,7 +83,7 @@ TagContentConverter should return correct values for valid input.
 As we are trying to test our "units" of code independently, we want to make sure that we do not rely objects and data generated by "external" code. If we we did, then we wouldn't know if the failure we're observing was caused by the code we are actually trying to test or something external to it.
 
 
-However, the features that we are trying to test often depend on those objects generated by external pieces of code. It would be difficult to test a bot command without having access to a `Context` instance. Fortunately, there's a solution for that: we use fake objects that act like the true object. We call these fake objects "mocks". 
+However, the features that we are trying to test often depend on those objects generated by external pieces of code. It would be difficult to test a bot command without having access to a `Context` instance. Fortunately, there's a solution for that: we use fake objects that act like the true object. We call these fake objects "mocks".
 
 To create these mock object, we mainly use the [`unittest.mock`](https://docs.python.org/3/library/unittest.mock.html) module. In addition, we have also defined a couple of specialized mock objects that mock specific `discord.py` types (see the section on the below.).
 
@@ -114,13 +114,13 @@ class BotCogTests(unittest.TestCase):
 
 ### Mocking coroutines
 
-By default, the `unittest.mock.Mock` and `unittest.mock.MagicMock` classes cannot mock coroutines, since the `__call__` method they provide is synchronous. In anticipation of the `AsyncMock` that will be [introduced in Python 3.8](https://docs.python.org/3.9/whatsnew/3.8.html#unittest), we have added an `AsyncMock` helper to [`helpers.py`](/tests/helpers.py). Do note that this drop-in replacement only implements an asynchronous `__call__` method, not the additional assertions that will come with the new `AsyncMock` type in Python 3.8. 
+By default, the `unittest.mock.Mock` and `unittest.mock.MagicMock` classes cannot mock coroutines, since the `__call__` method they provide is synchronous. In anticipation of the `AsyncMock` that will be [introduced in Python 3.8](https://docs.python.org/3.9/whatsnew/3.8.html#unittest), we have added an `AsyncMock` helper to [`helpers.py`](/tests/helpers.py). Do note that this drop-in replacement only implements an asynchronous `__call__` method, not the additional assertions that will come with the new `AsyncMock` type in Python 3.8.
 
 ### Special mocks for some `discord.py` types
 
 To quote Ned Batchelder, Mock objects are "automatic chameleons". This means that they will happily allow the access to any attribute or method and provide a mocked value in return. One downside to this is that if the code you are testing gets the name of the attribute wrong, your mock object will not complain and the test may still pass.
 
-In order to avoid that, we have defined a number of Mock types in [`helpers.py`](/tests/helpers.py) that follow the specifications of the actual Discord types they are mocking. This means that trying to access an attribute or method on a mocked object that does not exist on the equivalent `discord.py` object will result in an `AttributeError`. In addition, these mocks have some sensible defaults and **pass `isinstance` checks for the types they are mocking**. 
+In order to avoid that, we have defined a number of Mock types in [`helpers.py`](/tests/helpers.py) that follow the specifications of the actual Discord types they are mocking. This means that trying to access an attribute or method on a mocked object that does not exist on the equivalent `discord.py` object will result in an `AttributeError`. In addition, these mocks have some sensible defaults and **pass `isinstance` checks for the types they are mocking**.
 
 These special mocks are added when they are needed, so if you think it would be sensible to add another one, feel free to propose one in your PR.
 
@@ -144,7 +144,7 @@ Finally, there are some considerations to make when writing tests, both for writ
 
 ### Test coverage is a starting point
 
-Having test coverage is a good starting point for unit testing: If a part of your code was not covered by a test, we know that we have not tested it properly. The reverse is unfortunately not true: Even if the code we are testing has 100% branch coverage, it does not mean it's fully tested or guaranteed to work. 
+Having test coverage is a good starting point for unit testing: If a part of your code was not covered by a test, we know that we have not tested it properly. The reverse is unfortunately not true: Even if the code we are testing has 100% branch coverage, it does not mean it's fully tested or guaranteed to work.
 
 One problem is that 100% branch coverage may be misleading if we haven't tested our code against all the realistic input it may get in production. For instance, take a look at the following `member_information` function and the test we've written for it:
 
@@ -169,7 +169,7 @@ class FunctionsTests(unittest.TestCase):
 
 If you were to run this test, not only would the function pass the test, `coverage.py` will also tell us that the test provides 100% branch coverage for the function. Can you spot the bug the test suite did not catch?
 
-The problem here is that we have only tested our function with a member object that had `None` for the `member.joined` attribute. This means that `member.joined.stfptime("%d-%m-%Y")` was never executed during our test, leading to us missing the spelling mistake in `stfptime` (it should be `strftime`). 
+The problem here is that we have only tested our function with a member object that had `None` for the `member.joined` attribute. This means that `member.joined.stfptime("%d-%m-%Y")` was never executed during our test, leading to us missing the spelling mistake in `stfptime` (it should be `strftime`).
 
 Adding another test would not increase the test coverage we have, but it does ensure that we'll notice that this function can fail with realistic data:
 
diff --git a/tests/base.py b/tests/base.py
index 42174e911..d99b9ac31 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -31,7 +31,7 @@ class LoggingTestsMixin:
     """
 
     @contextmanager
-    def assertNotLogs(self, logger=None, level=None, msg=None):
+    def assertNotLogs(self, logger=None, level=None, msg=None):  # noqa: N802
         """
         Asserts that no logs of `level` and higher were emitted by `logger`.
 
@@ -81,7 +81,7 @@ class LoggingTestsMixin:
 class CommandTestCase(unittest.IsolatedAsyncioTestCase):
     """TestCase with additional assertions that are useful for testing Discord commands."""
 
-    async def assertHasPermissionsCheck(
+    async def assertHasPermissionsCheck(  # noqa: N802
         self,
         cmd: commands.Command,
         permissions: Dict[str, bool],
diff --git a/tests/bot/cogs/sync/test_base.py b/tests/bot/cogs/sync/test_base.py
index fe0594efe..6ee9dfda6 100644
--- a/tests/bot/cogs/sync/test_base.py
+++ b/tests/bot/cogs/sync/test_base.py
@@ -84,7 +84,7 @@ class SyncerSendPromptTests(unittest.IsolatedAsyncioTestCase):
 
                 method.assert_called_once_with(constants.Channels.dev_core)
 
-    async def test_send_prompt_returns_None_if_channel_fetch_fails(self):
+    async def test_send_prompt_returns_none_if_channel_fetch_fails(self):
         """None should be returned if there's an HTTPException when fetching the channel."""
         self.bot.get_channel.return_value = None
         self.bot.fetch_channel.side_effect = discord.HTTPException(mock.MagicMock(), "test error!")
diff --git a/tests/bot/cogs/test_cogs.py b/tests/bot/cogs/test_cogs.py
new file mode 100644
index 000000000..39f6492cb
--- /dev/null
+++ b/tests/bot/cogs/test_cogs.py
@@ -0,0 +1,80 @@
+"""Test suite for general tests which apply to all cogs."""
+
+import importlib
+import pkgutil
+import typing as t
+import unittest
+from collections import defaultdict
+from types import ModuleType
+from unittest import mock
+
+from discord.ext import commands
+
+from bot import cogs
+
+
+class CommandNameTests(unittest.TestCase):
+    """Tests for shadowing command names and aliases."""
+
+    @staticmethod
+    def walk_commands(cog: commands.Cog) -> t.Iterator[commands.Command]:
+        """An iterator that recursively walks through `cog`'s commands and subcommands."""
+        # Can't use Bot.walk_commands() or Cog.get_commands() cause those are instance methods.
+        for command in cog.__cog_commands__:
+            if command.parent is None:
+                yield command
+                if isinstance(command, commands.GroupMixin):
+                    # Annoyingly it returns duplicates for each alias so use a set to fix that
+                    yield from set(command.walk_commands())
+
+    @staticmethod
+    def walk_modules() -> t.Iterator[ModuleType]:
+        """Yield imported modules from the bot.cogs subpackage."""
+        def on_error(name: str) -> t.NoReturn:
+            raise ImportError(name=name)
+
+        # The mock prevents asyncio.get_event_loop() from being called.
+        with mock.patch("discord.ext.tasks.loop"):
+            for module in pkgutil.walk_packages(cogs.__path__, "bot.cogs.", onerror=on_error):
+                if not module.ispkg:
+                    yield importlib.import_module(module.name)
+
+    @staticmethod
+    def walk_cogs(module: ModuleType) -> t.Iterator[commands.Cog]:
+        """Yield all cogs defined in an extension."""
+        for obj in module.__dict__.values():
+            # Check if it's a class type cause otherwise issubclass() may raise a TypeError.
+            is_cog = isinstance(obj, type) and issubclass(obj, commands.Cog)
+            if is_cog and obj.__module__ == module.__name__:
+                yield obj
+
+    @staticmethod
+    def get_qualified_names(command: commands.Command) -> t.List[str]:
+        """Return a list of all qualified names, including aliases, for the `command`."""
+        names = [f"{command.full_parent_name} {alias}".strip() for alias in command.aliases]
+        names.append(command.qualified_name)
+
+        return names
+
+    def get_all_commands(self) -> t.Iterator[commands.Command]:
+        """Yield all commands for all cogs in all extensions."""
+        for module in self.walk_modules():
+            for cog in self.walk_cogs(module):
+                for cmd in self.walk_commands(cog):
+                    yield cmd
+
+    def test_names_dont_shadow(self):
+        """Names and aliases of commands should be unique."""
+        all_names = defaultdict(list)
+        for cmd in self.get_all_commands():
+            func_name = f"{cmd.module}.{cmd.callback.__qualname__}"
+
+            for name in self.get_qualified_names(cmd):
+                with self.subTest(cmd=func_name, name=name):
+                    if name in all_names:
+                        conflicts = ", ".join(all_names.get(name, ""))
+                        self.fail(
+                            f"Name '{name}' of the command {func_name} conflicts with {conflicts}."
+                        )
+
+                all_names[name].append(func_name)
diff --git a/tests/bot/cogs/test_information.py b/tests/bot/cogs/test_information.py
index 5693d2946..3c26374f5 100644
--- a/tests/bot/cogs/test_information.py
+++ b/tests/bot/cogs/test_information.py
@@ -45,10 +45,9 @@ class InformationCogTests(unittest.TestCase):
         _, kwargs = self.ctx.send.call_args
         embed = kwargs.pop('embed')
 
-        self.assertEqual(embed.title, "Role information")
+        self.assertEqual(embed.title, "Role information (Total 1 role)")
         self.assertEqual(embed.colour, discord.Colour.blurple())
-        self.assertEqual(embed.description, f"`{self.moderator_role.id}` - {self.moderator_role.mention}\n")
-        self.assertEqual(embed.footer.text, "Total roles: 1")
+        self.assertEqual(embed.description, f"\n`{self.moderator_role.id}` - {self.moderator_role.mention}\n")
 
     def test_role_info_command(self):
         """Tests the `role info` command."""
diff --git a/tests/bot/cogs/test_snekbox.py b/tests/bot/cogs/test_snekbox.py
index 9cd7f0154..1dec0ccaf 100644
--- a/tests/bot/cogs/test_snekbox.py
+++ b/tests/bot/cogs/test_snekbox.py
@@ -1,11 +1,13 @@
 import asyncio
 import logging
 import unittest
-from unittest.mock import AsyncMock, MagicMock, Mock, call, patch
+from unittest.mock import AsyncMock, MagicMock, Mock, call, create_autospec, patch
 
+from discord.ext import commands
+
+from bot import constants
 from bot.cogs import snekbox
 from bot.cogs.snekbox import Snekbox
-from bot.constants import URLs
 from tests.helpers import MockBot, MockContext, MockMessage, MockReaction, MockUser
 
 
@@ -23,7 +25,7 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase):
 
         self.assertEqual(await self.cog.post_eval("import random"), "return")
         self.bot.http_session.post.assert_called_with(
-            URLs.snekbox_eval_api,
+            constants.URLs.snekbox_eval_api,
             json={"input": "import random"},
             raise_for_status=True
         )
@@ -43,10 +45,10 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase):
 
         self.assertEqual(
             await self.cog.upload_output("My awesome output"),
-            URLs.paste_service.format(key=key)
+            constants.URLs.paste_service.format(key=key)
         )
         self.bot.http_session.post.assert_called_with(
-            URLs.paste_service.format(key="documents"),
+            constants.URLs.paste_service.format(key="documents"),
             data="My awesome output",
             raise_for_status=True
         )
@@ -89,15 +91,15 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase):
                 self.assertEqual(actual, expected)
 
     @patch('bot.cogs.snekbox.Signals', side_effect=ValueError)
-    def test_get_results_message_invalid_signal(self, mock_Signals: Mock):
+    def test_get_results_message_invalid_signal(self, mock_signals: Mock):
         self.assertEqual(
             self.cog.get_results_message({'stdout': '', 'returncode': 127}),
             ('Your eval job has completed with return code 127', '')
         )
 
     @patch('bot.cogs.snekbox.Signals')
-    def test_get_results_message_valid_signal(self, mock_Signals: Mock):
-        mock_Signals.return_value.name = 'SIGTEST'
+    def test_get_results_message_valid_signal(self, mock_signals: Mock):
+        mock_signals.return_value.name = 'SIGTEST'
         self.assertEqual(
             self.cog.get_results_message({'stdout': '', 'returncode': 127}),
             ('Your eval job has completed with return code 127 (SIGTEST)', '')
@@ -279,11 +281,14 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase):
         """Test that the continue_eval function does continue if required conditions are met."""
         ctx = MockContext(message=MockMessage(add_reaction=AsyncMock(), clear_reactions=AsyncMock()))
         response = MockMessage(delete=AsyncMock())
-        new_msg = MockMessage(content='!e NewCode')
+        new_msg = MockMessage()
         self.bot.wait_for.side_effect = ((None, new_msg), None)
+        expected = "NewCode"
+        self.cog.get_code = create_autospec(self.cog.get_code, spec_set=True, return_value=expected)
 
         actual = await self.cog.continue_eval(ctx, response)
-        self.assertEqual(actual, 'NewCode')
+        self.cog.get_code.assert_awaited_once_with(new_msg)
+        self.assertEqual(actual, expected)
         self.bot.wait_for.assert_has_awaits(
             (
                 call('message_edit', check=partial_mock(snekbox.predicate_eval_message_edit, ctx), timeout=10),
@@ -302,6 +307,32 @@ class SnekboxTests(unittest.IsolatedAsyncioTestCase):
         self.assertEqual(actual, None)
         ctx.message.clear_reactions.assert_called_once()
 
+    async def test_get_code(self):
+        """Should return 1st arg (or None) if eval cmd in message, otherwise return full content."""
+        prefix = constants.Bot.prefix
+        subtests = (
+            (self.cog.eval_command, f"{prefix}{self.cog.eval_command.name} print(1)", "print(1)"),
+            (self.cog.eval_command, f"{prefix}{self.cog.eval_command.name}", None),
+            (MagicMock(spec=commands.Command), f"{prefix}tags get foo"),
+            (None, "print(123)")
+        )
+
+        for command, content, *expected_code in subtests:
+            if not expected_code:
+                expected_code = content
+            else:
+                [expected_code] = expected_code
+
+            with self.subTest(content=content, expected_code=expected_code):
+                self.bot.get_context.reset_mock()
+                self.bot.get_context.return_value = MockContext(command=command)
+                message = MockMessage(content=content)
+
+                actual_code = await self.cog.get_code(message)
+
+                self.bot.get_context.assert_awaited_once_with(message)
+                self.assertEqual(actual_code, expected_code)
+
     def test_predicate_eval_message_edit(self):
         """Test the predicate_eval_message_edit function."""
         msg0 = MockMessage(id=1, content='abc')
diff --git a/tests/bot/test_utils.py b/tests/bot/test_utils.py
deleted file mode 100644
index d7bcc3ba6..000000000
--- a/tests/bot/test_utils.py
+++ /dev/null
@@ -1,37 +0,0 @@
-import unittest
-
-from bot import utils
-
-
-class CaseInsensitiveDictTests(unittest.TestCase):
-    """Tests for the `CaseInsensitiveDict` container."""
-
-    def test_case_insensitive_key_access(self):
-        """Tests case insensitive key access and storage."""
-        instance = utils.CaseInsensitiveDict()
-
-        key = 'LEMON'
-        value = 'trees'
-
-        instance[key] = value
-        self.assertIn(key, instance)
-        self.assertEqual(instance.get(key), value)
-        self.assertEqual(instance.get(key.casefold()), value)
-        self.assertEqual(instance.pop(key.casefold()), value)
-        self.assertNotIn(key, instance)
-        self.assertNotIn(key.casefold(), instance)
-
-        instance.setdefault(key, value)
-        del instance[key]
-        self.assertNotIn(key, instance)
-
-    def test_initialization_from_kwargs(self):
-        """Tests creating the dictionary from keyword arguments."""
-        instance = utils.CaseInsensitiveDict({'FOO': 'bar'})
-        self.assertEqual(instance['foo'], 'bar')
-
-    def test_update_from_other_mapping(self):
-        """Tests updating the dictionary from another mapping."""
-        instance = utils.CaseInsensitiveDict()
-        instance.update({'FOO': 'bar'})
-        self.assertEqual(instance['foo'], 'bar')