Skip to content

Migrating the test suite to the unittest framework#517

Merged
lemonsaurus merged 36 commits into
masterfrom
unittest-migration
Oct 20, 2019
Merged

Migrating the test suite to the unittest framework#517
lemonsaurus merged 36 commits into
masterfrom
unittest-migration

Conversation

@SebastiaanZ
Copy link
Copy Markdown
Contributor

@SebastiaanZ SebastiaanZ commented Oct 11, 2019

edit
This is now ready for review. It's pretty massive, but a decent amount of the test files have already been reviewed.

For context:

This is the first phase of getting a nice test coverage for the bot, but there's still a long way to go.

Most of the specific test files have been migrated "as-is", without a lot of changes to their internal testing logic. If you feel that certain tests can be enhanced, feel free to make note of that, but we may postpone the implementation of those enhancements to after this is merged, so we can start on writing tests for the files that currently have no coverage as well.

Once this is merged, I will make (a lot of) issues to coordinate the process of adding more tests and test enhancements.


Old PR test
We recently decided to migrate the bot's test suite to the unittest framework. One of the main reasons for that decision was that one of our other core repositories, the site repository, uses the test tools Django provides, which are based on unittest. Since we want to have some consistency between our repositories, we decided that it would be better to use unittest for the bot as well.

This draft pull request is the start of that migration and will be our main way of keeping track of the progress we make towards providing a high test coverage for bot.

If you want to contribute to the migration, feel free to make a pull request to this branch. I've currently migrated four of the pytest test files we had to unittest (mostly as-is, I did not critically look at the files to see if they need any improvements) and there's an "in-progress" getting started guide for writing tests for our bot in the tests subdirectory of the unittest-migration branch. Together, they should give you a rough idea on how to get started.

Files on master that still need to be migrated:

  • ├── cogs
  • │   ├── sync
  • │   │   ├── __init__.py
  • │   │   ├── test_roles.py
  • │   │   └── test_users.py
  • │   ├── __init__.py
  • │   ├── test_antispam.py
  • │   ├── test_information.py
  • │   ├── test_security.py
  • │   └── test_token_remover.py
  • ├── rules
  • │   ├── __init__.py
  • │   └── test_attachments.py
  • ├── utils
  • │   ├── init.py
  • │   └── test_checks.py
  • ├── conftest.py
  • ├── helpers.py
  • ├── __init__.py
  • ├── test_api.py
  • ├── test_constants.py
  • ├── test_converters.py
  • ├── test_pagination.py
  • └── test_resources.py

In addition, there are plenty of files in bot that currently do not have any testing at all.

After a discussion in the core developers channel, we have decided to
migrate from `pytest` to `unittest` as the testing framework. This
commit sets up the repository to use `unittest` and migrates the
first couple of tests files to the new framework.

What I have done to migrate to `unitest`:

- Removed all `pytest` test files, since they are incompatible.

- Removed `pytest`-related dependencies from the Pipfile.

- Added `coverage.py` to the Pipfile dev-packages and relocked.

- Added convenience scripts to Pipfile for running the test suite.

- Adjust to `azure-pipelines.yml` to use `coverage.py` and `unittest`.

- Migrated four test files from `pytest` to `unittest` format.

In addition, I've added five helper Mock subclasses in `helpers.py`
and created a `TestCase` subclass in `base.py` to add an assertion
that asserts that no log records were logged within the context of
the context manager. Obviously, these new utility functions and
classes are fully tested in their respective `test_` files.

Finally, I've started with an introductory guide for writing tests
for our bot in `README.md`.
I forgot to test some aspects of the `tests.base` module, including
some branches of the `self.assertNotLogs` method. I've corrected that
by including a couple of tests.

I also removed the test result publishing from the Azure pipeline,
since I've not configured an XML test runner yet. The coverage report
is still published, of course and test output will be available in
standard out, so information is readily available.
I have change the testrunner from `unittest` to `xmlrunner` in the
Azure pipeline to be able to publish our test results on Azure. This
is the same runner as `site` uses to generate XML reports.

In addition, I've cleaned up some small mistakes in docstrings and
`README.md`.
I've made some textual changes to the testing guidelines defined
in README.md.
@SebastiaanZ SebastiaanZ added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) type: enhancement labels Oct 11, 2019
@SebastiaanZ SebastiaanZ changed the title Migration the test suite to the unittest framework Migrating the test suite to the unittest framework Oct 11, 2019
@kraktus
Copy link
Copy Markdown
Contributor

kraktus commented Oct 12, 2019

Nice readme

@jchristgit jchristgit self-assigned this Oct 12, 2019
@jchristgit
Copy link
Copy Markdown
Contributor

In addition, there are plenty of files in bot that currently do not have any testing at all.

I'm gonna open PRs for the remaining tests to be moved - and I'll get back to adding new tests afterwards.

SebastiaanZ and others added 11 commits October 14, 2019 14:16
Migrates the `test_constants.py` file to unittest. As with the pytest
version, there is not yet support to test container types.
This commit introduces some new Mock-types to the already existing
Mock-types for discord.py objects. The total list is now:

- MockGuild
- MockRole
- MockMember
- MockBot
- MockContext
- MockTextChannel
- MockMessage

In addition, I've added all coroutines in the documentation for these
discord.py objects as `AsyncMock` attributes to ease testing. Tests
ensure that the attributes set for the Mocks exist for the actual
discord.py objects as well.
This commit replaces the standard MagicMocks by our specialized mocks
for discord.py objects. It also adds the missing `channel` attribute
to the `tests.helpers.MockMessage` mock and moves the file to the
correct folder.
Resolving merge conflicts from master in `.gitignore` and
`tests/helpers.py`.
@SebastiaanZ SebastiaanZ marked this pull request as ready for review October 15, 2019 13:57
@SebastiaanZ
Copy link
Copy Markdown
Contributor Author

This is ready for review. It's pretty massive, but a decent amount of the test files have already been reviewed.

For context:

This is the first phase of getting a nice test coverage for the bot, but there's still a long way to go.

Most of the specific test files have been migrated "as-is", without a lot of changes to their internal testing logic. If you feel that certain tests can be enhanced, feel free to make note of that, but we may postpone the implementation of those enhancements to after this is merged, so we can start on writing tests for the files that currently have no coverage as well.

Once this is merged, I will make (a lot of) issues to coordinate the process of adding more tests and test enhancements.

@jchristgit jchristgit self-requested a review October 19, 2019 15:44
Copy link
Copy Markdown
Contributor

@jchristgit jchristgit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for taking so long, this looks great!

Copy link
Copy Markdown
Contributor

@lemonsaurus lemonsaurus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I haven't found the time to do a really deep dive into every single one of the 40+ files in this PR, the overall quality of this PR is absolutely excellent. Clever solutions, succinct Python and clean solutions all around.

Provided that this has been tested properly, I think we should get it merged.

coroutine = self.cog.role_info.callback(self.cog, self.ctx, dummy_role, admin_role)

self.assertIsNone(asyncio.run(coroutine))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this section is a little needlessly newliney


self.assertEqual(self.ctx.send.call_count, 2)

(_, dummy_kwargs), (_, admin_kwargs) = self.ctx.send.call_args_list
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could probably do with a comment, it looks a bit arcane. had to do a double take.

@lemonsaurus lemonsaurus merged commit f194df8 into master Oct 20, 2019
@lemonsaurus lemonsaurus deleted the unittest-migration branch October 20, 2019 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants