Enhancements for tests.helpers and our test suite#660
Merged
Conversation
Previously, logging messages would output to std.out. when running individual test files (instead of running the entire suite). To prevent this, I've added a `for`-loop to `tests.helpers` that sets the level of all registered loggers to `CRITICAL`. The reason for adding this to `tests.helpers` is simple: It's the most common file to be imported in individual tests, increasing the chance of the code being run for individual test files. A small downside of this way of handling logging is that when we are trying to assert logging messages are being emitted, we need to set the logger explicitly in the `self.assertLogs` context manager. This is a small downside, though, and probably good practice anyway. There was one test in `tests.bot.test_api` that did not do this, so I have changed this to make the test compatible with the new set-up.
The `name` keyword argument has a special meaning for the default mockobjects provided by `unittest.mock`. This means that by default, the common d.py `name` attribute can't be set during initalization of one of our custom Mock-objects by passing it to the constructor. Since it's unlikely for us to make use of the special `name` feature of mocks and more likely to want to set the d.py `name` attribute, I added special handling of the `name` kwarg.
Our custom `discord.py` now follow the specifications of the object they are mocking more strictly by using the `spec_set` instead of the `spec` kwarg to initialize the specifications. This means that trying to set an attribute that does not follow the specifications will now also result in an `AttributeError`. To make sure we are not trying to set illegal attributes during the default initialization of the mock objects, I've changed the way we handle default values of parameters. This does introduce a breaking change: Instead of passing a `suffix_id`, the `id` attribute should now be passed using the exact name. `id`. This commit also makes sure existing tests follow this change.
Previously, the coroutine object passed to `MockBot.loop.create_task` would trigger a `RuntimeWarning` for not being awaited as we do not actually create a task for it. To prevent these warnings, coroutine objects passed will now automatically be closed.
lemonsaurus
approved these changes
Nov 13, 2019
Contributor
lemonsaurus
left a comment
There was a problem hiding this comment.
Spectacular work. Let's do it.
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several enhancements to our testing suite to make running tests easier and reduce the number of developer surprises. It does come with a couple of breaking changes that may influence currently open PRs (I haven't checked yet, but they should be easy to resolve).
Enhancements
Stricter custom mock types
One of the reasons for using custom mock types is because they follow the specifications of the actual objects they're mocking. However, as @kwzrd remarked, the
specmethod we used for that only provided partial protection:specdoes not prevent setting attributes that should not exist on the object we are mocking. I have solved this by using the stricterspec_setoption that makes sure the validity of attributes is also checked when they are set (as opposed to only when they are accessed).Prevent logging when running individual tests
As you may have noticed, logging messages were not suppressed when running an individual test file (instead of the entire test suite), making the output extremely messy. This is now no longer the case.
Prevent RuntimeWarning after
MockBot.loop.create_taskWhen "scheduling" a coroutine object with
MockBot.loop.create_task, the coroutine object is not actually scheduled as a task since we're mocking thecreate_taskmethod and we're not actually running an event loop. While this is typically what we want, this triggersRuntimeWarningssince the coroutine object was never awaited. I've solved this by setting a defaultside_effectforMockBot.loop.create_taskthat callsstop()on the passed coroutine object.Allow
nameattribute to be set with__init__The
namekwarg has a special meaning in the__init__methods ofMockandMagicMockpreventing us from setting anameattribute during initialization of our custom mock types that inherit from these classes. This is not desirable, asdiscord.pyobjects frequently have anameattribute and we want to be able to set it when initializing the mock. I've added special handling of thenamekwarg to prevent it from being passed to the__init__of the parent classes.Note: This does prevent us from using that special
namehandling of Mock/MagicMock, but I don't think that option is useful for us in the first place. Being able to set thenameatttribute of the mock is much more useful and actually in use a lot in our current tests.Use actual attribute names instead of 'safe' surrogates
Previously, I used surrogates like
message_idto set theidattribute of mocks in__init__to prevent eclipsing the built-inid. Since I've now switched tokwargs-only approach in the__init__, this is no longer necessary, as no eclipsing will occur. This means we can now just pass the attributes by their actual name to prevent "developer surprise". (My apologies for those hours of frustration, @lemonsaurus...)Allow all attributes to be set during initialization of the mock
Due to a mistake, some attributes could not be set during initialization of a custom mock type as the attributes were overwritten by default values. This is no longer the case.
Breaking changes
name="Helpers").user_idormessage_idno longer work and will result in anAttributeErrorsince they are not valid attributes of the types being mocked.