Add unit tests #161

super-cooper · 2023-06-07T11:05:22Z

This patch fully implements pytest-based unit tests for memebot. It currently tests all the commands plus Twitter integration. Most of the tests are fairly thorough.

In order for the tests to run correctly, Memebot's code has been moved from the src directory to a root memebot package, as is typical for Python projects. The docker-compose file and README have been updated to reflect this as well.

Resolves #31

Python projects typically do not have a `src` directory. The code is typically all located in a root pacakge under the same name as the project.

* Remove extraneous `pytest` install * Remove unused secrets * Use more detailed test runner output

With mocking, type checking becomes very difficult and annoying

nhawke

When trying to run this, i ran into an issue trying to install emoji 2.0 (per requirements.txt) but i was able to install 2.8. We might need to bunp that version in the requirements.

nhawke · 2023-11-09T00:49:51Z

memebot/lib/util.py

 if TYPE_CHECKING:
-    from discord.types.interactions import InteractionData
+    pass


You can just remove this if entirely we don't need the type-check-specific import

nhawke · 2023-11-09T00:51:23Z

memebot/main.py


-import config
-import memebot
+config.populate_config_from_command_line()


Does this need to go before the other imports or in main.py at all? Could this be an import side effect by putting this in the config module's init.py (or wherever's most appropriate)?

Modified the client to be created lazily on first use, rather than at module import. Should prevent us from having to worry about side effects, similar to what was done with the logging package.

nhawke · 2023-11-09T01:07:41Z

tests/commands/test_hello.py

+    await commands.hello.callback(mock_interaction)
+
+    mock_interaction.response.send_message.assert_awaited_once_with(
+        f"Hello, {mock_interaction.user.display_name}!"


I dont think this is right. The Mock here is empty, so the mock_interaction.user.display_name will also be empty. While this test technically produces the right result, I think it would be better to populate that field so we can ensure the sent message actually accesses the property rather than something else that gives a None.

I believe the name would have been a unique MagicMock, but set it to a string.

nhawke · 2023-11-09T01:14:57Z

tests/commands/test_poll.py

+@pytest.mark.parametrize(
+    ["a1", "a2", "a3", "a4", "a5"],
+    # Create a matrix of all possible answer inputs that contain only 1 real answer
+    [["Mock answer" if i == j else None for i in range(5)] for j in range(5)],


Can you expand this out so the test cases are more explicit?

Done. (Thanks ChatGPT)

nhawke · 2023-11-09T01:31:11Z

README.md

+The tests can also be run from the _test_ Docker image:
+
+```shell
+$ docker run --rm -it --entrypoint python3 memebot:test -m pytest [/path/to/test/package/or/module]


This isn't running all the tests for me:

$ docker run --rm -it --entrypoint python3 memebot:test -m pytest ================================================= test session starts ================================================== platform linux -- Python 3.9.6, pytest-7.1.2, pluggy-1.0.0 rootdir: /opt/memebot collected 1 item tests/test_pytest.py . [100%] ================================================== 1 passed in 0.01s ===================================================

Fixed by copying the tests into the test image

Instead of creating the client on load of `client.py` module, the client is instead instantiated with the first call to a new function `get_memebot()`. This eliminates side effects related to module load order.

nhawke · 2024-09-09T16:14:15Z

tests/commands/test_poll.py

+        [None, None, "Mock answer", None, None],
+        [None, None, None, "Mock answer", None],
+        [None, None, None, None, "Mock answer"],
+    ],


Each of these should be parameters at the top level, not nested inside an array.

Also you can lose the comment because the cases are self-explanatory now that the list comp is gone.

Each of these should be parameters at the top level, not nested inside an array.

What is meant by this? pytest.mark.parameterize accepts a list of parameter configurations for the test.

Oh my bad, parameterize just has a really stupid interface. Shoulda known better than to assume anything in python testing was sane...

nhawke · 2024-09-09T16:16:23Z

tests/commands/test_role.py

+async def do_role_test(
+    mock_interaction: mock.Mock,
+    mock_guild: Optional[mock.Mock],
+    command: discord.ext.commands.Command,
+    *args: Any,
+    **kwargs: Any,
+) -> None:
+    """
+    Helper function for common role test pattern
+    """
+    mock_interaction.guild = mock_guild
+
+    await command.callback(mock_interaction, *args, **kwargs)
+    mock_interaction.response.send_message.assert_awaited_once()


This doesn't need to be a helper. Better to explicitly do this in each case that uses it so it's more obvious what's happening in the test.

nhawke · 2024-09-09T16:17:20Z

tests/commands/test_role.py

+    """
+    Test failure to create a role with an invalid name
+    """
+    await do_role_create_failure(


There shouldn't be this many layers of indirection for a test. It makes it hard to understand what the test is doing. Better for test logic to be repetitive but explicit.

Same goes for the other tests in this file. Helpers would make sense for test setup but for the critical logic section of the test it should just be explicity in the test funciton.

This is why I hate writing tests.

nhawke · 2024-09-09T16:20:37Z

tests/fixtures/discord_fixtures.py

Are this and the twitter fixture actually used anywhere? I might just be missing something but I don't see the usage.

They are. Fixtures are a pytest feature and they get imported and used automatically. For example, the fixture defined by

def mock_discord_client(MockClient: type[mock.Mock]) -> mock.Mock: return MockClient()

gets called and the return value passed to every test or fixture which has a parameter with the name mock_discord_client. It's a simple way of defining repeatable test fixtures without needing to call helper functions to use them all in every test.

I love implicit behavior and mocks !! Python tests and readability remain enemies

nhawke · 2024-09-09T16:22:45Z

tests/commands/test_poll.py

+
+    mock_interaction.response.send_message.assert_awaited_once()
+    do_poll_embed_test(mock_interaction, question, expected_fields)
+    await do_poll_votes_test(mock_interaction, [], yes_no=True)


Can this test be combined with the other test which uses these helpers? then the helpers can just be inlined to avoid test logic indireciton.

I'd rather not. The test entrypoints exist to clarify what is being tested. If we combined them, we'd just need to gate it all with if-statements anyway. I think this is fine. I'm also gonna delete the poll command and all the tests after we merge this lmao

I disagree but moot anyway

super-cooper

Addressed issues. Dependency stuff can get resolved after this is merged. Could be a python version incompatibility on your end, but once tests are merged, I can enable dependabot for Python packages which should handle issues like this going forward.

super-cooper · 2024-09-09T22:30:43Z

tests/commands/test_poll.py

+        [None, None, "Mock answer", None, None],
+        [None, None, None, "Mock answer", None],
+        [None, None, None, None, "Mock answer"],
+    ],


Each of these should be parameters at the top level, not nested inside an array.

What is meant by this? pytest.mark.parameterize accepts a list of parameter configurations for the test.

super-cooper · 2024-09-09T22:36:06Z

tests/fixtures/discord_fixtures.py

They are. Fixtures are a pytest feature and they get imported and used automatically. For example, the fixture defined by

def mock_discord_client(MockClient: type[mock.Mock]) -> mock.Mock: return MockClient()

gets called and the return value passed to every test or fixture which has a parameter with the name mock_discord_client. It's a simple way of defining repeatable test fixtures without needing to call helper functions to use them all in every test.

super-cooper · 2024-11-12T22:06:21Z

tests/commands/test_poll.py

+
+    mock_interaction.response.send_message.assert_awaited_once()
+    do_poll_embed_test(mock_interaction, question, expected_fields)
+    await do_poll_votes_test(mock_interaction, [], yes_no=True)


I'd rather not. The test entrypoints exist to clarify what is being tested. If we combined them, we'd just need to gate it all with if-statements anyway. I think this is fine. I'm also gonna delete the poll command and all the tests after we merge this lmao

super-cooper · 2024-11-12T22:24:49Z

tests/commands/test_role.py

+async def do_role_test(
+    mock_interaction: mock.Mock,
+    mock_guild: Optional[mock.Mock],
+    command: discord.ext.commands.Command,
+    *args: Any,
+    **kwargs: Any,
+) -> None:
+    """
+    Helper function for common role test pattern
+    """
+    mock_interaction.guild = mock_guild
+
+    await command.callback(mock_interaction, *args, **kwargs)
+    mock_interaction.response.send_message.assert_awaited_once()


super-cooper · 2024-11-12T22:25:03Z

tests/commands/test_role.py

+    """
+    Test failure to create a role with an invalid name
+    """
+    await do_role_create_failure(


This is why I hate writing tests.

nhawke

Looks python to me

nhawke · 2024-11-13T00:08:38Z

tests/commands/test_poll.py

+
+    mock_interaction.response.send_message.assert_awaited_once()
+    do_poll_embed_test(mock_interaction, question, expected_fields)
+    await do_poll_votes_test(mock_interaction, [], yes_no=True)


I disagree but moot anyway

nhawke · 2024-11-13T00:10:52Z

tests/commands/test_poll.py

+        [None, None, "Mock answer", None, None],
+        [None, None, None, "Mock answer", None],
+        [None, None, None, None, "Mock answer"],
+    ],


Oh my bad, parameterize just has a really stupid interface. Shoulda known better than to assume anything in python testing was sane...

nhawke · 2024-11-13T00:13:09Z

tests/fixtures/discord_fixtures.py

I love implicit behavior and mocks !! Python tests and readability remain enemies

super-cooper added 12 commits June 7, 2023 05:38

Move Memebot code to memebot package

fb834f3

Python projects typically do not have a `src` directory. The code is typically all located in a root pacakge under the same name as the project.

Update test requirements to support new tests

3d6e720

Update docker-compose.yaml and README to reflect new memebot package

3129334

Update Dockerfile to use new memebot package

d61e6bf

Add initial test fixtures

7e0b103

Add tests for /hello command

fe0de15

Add tests for /poll command

f936cc8

Add tests for /role command

57d626c

Add tests for Twitter integration

57a89b8

Update mypy config to ignore discord.ext.commands

9e29794

Update pytest workflow

29789c6

* Remove extraneous `pytest` install * Remove unused secrets * Use more detailed test runner output

Update README with instructions on running tests

3933fdf

super-cooper self-assigned this Jun 7, 2023

super-cooper requested a review from nhawke June 7, 2023 11:05

super-cooper added 5 commits June 7, 2023 06:09

Update pytest action to remove unnecessary permission

5df38fb

Fix lint

1540442

Exclude tests from mypy

328b108

With mocking, type checking becomes very difficult and annoying

Rename pytest job

c399be5

Fix broken pytest job

0bea15f

nhawke requested changes Nov 9, 2023

View reviewed changes

super-cooper added 7 commits May 5, 2024 20:21

Lazily instantiate Discord client

4a7be89

Instead of creating the client on load of `client.py` module, the client is instead instantiated with the first call to a new function `get_memebot()`. This eliminates side effects related to module load order.

Run all pytests by default

821e07e

Use constant display name in test for hello.py

48aa925

Use static answer matrix in single-answer test for poll.py

fbc3993

Remove redundant check for typing module

19179ad

Remove stale script entry from .dockerignore

7cd0adf

Fix lint

4bfd634

super-cooper requested a review from nhawke May 6, 2024 00:29

This was referenced Sep 3, 2024

Remove Twitter integration #218

Open

Remove /poll command #219

Open

nhawke requested changes Sep 9, 2024

View reviewed changes

Remove helpers from /role tests

74ff612

super-cooper commented Nov 12, 2024

View reviewed changes

Fix failing /role tests

8470738

super-cooper requested a review from nhawke November 12, 2024 23:04

nhawke approved these changes Nov 13, 2024

View reviewed changes

super-cooper merged commit 98b18b8 into master Nov 13, 2024
11 checks passed

super-cooper deleted the unit-test branch November 13, 2024 01:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests #161

Add unit tests #161

super-cooper commented Jun 7, 2023

nhawke left a comment

nhawke Nov 9, 2023

nhawke Nov 9, 2023

super-cooper May 5, 2024

nhawke Nov 9, 2023

super-cooper May 6, 2024

nhawke Nov 9, 2023

super-cooper May 6, 2024

nhawke Nov 9, 2023

super-cooper May 6, 2024

nhawke Sep 9, 2024

super-cooper Sep 9, 2024

nhawke Nov 13, 2024

nhawke Sep 9, 2024

super-cooper Nov 12, 2024

nhawke Sep 9, 2024

super-cooper Nov 12, 2024

nhawke Sep 9, 2024

super-cooper Sep 9, 2024

nhawke Nov 13, 2024

nhawke Sep 9, 2024

super-cooper Nov 12, 2024

nhawke Nov 13, 2024

super-cooper left a comment

super-cooper Sep 9, 2024

super-cooper Sep 9, 2024

super-cooper Nov 12, 2024

super-cooper Nov 12, 2024

super-cooper Nov 12, 2024

nhawke left a comment

nhawke Nov 13, 2024

nhawke Nov 13, 2024

nhawke Nov 13, 2024

Add unit tests #161

Add unit tests #161

Conversation

super-cooper commented Jun 7, 2023

nhawke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

super-cooper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhawke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment