Enhancements and a bug fix for our test helpers#629
Merged
Conversation
- https://docs.python.org/3/library/unittest.mock.html We previously used an override of the `__new__` method to prevent our custom mock types from instantiating their children with their own type instead of a general mock type like `MagicMock` or `Mock`. As it turns out, the Python documentation suggests another method of doing this that does not involve overriding `__new__`. This commit implements this new method to make sure we're using the idiomatic way of handling this. The suggested method is overriding the `_get_child_mock` method in the subclass. To make our code DRY, I've created a mixin that should come BEFORE the mock type we're subclassing in the MRO. --- In addition, I have also added this new mixin to our `AsyncMock` class to make sure that its `__call__` method returns a proper mock object after it has been awaited. This makes sure that subsequent attribute access on the returned object is mocked as expected.
I have enhanced the custom mocks defined in `tests/helpers.py` in a
couple of important ways.
1. Automatically create AsyncMock attributes using `inspect`
Our previous approach, hard-coding AsynckMock attributes for all the
coroutine function methods defined for the class we are trying to
mock is prone to human error and not resilient against changes
introduced in updates of the library we are using.
Instead, I have now created a helper method in our `CustomMockMixin`
(formerly `GetChildMockMixin`) that automatically inspects the spec
instance we've passed for `coroutine functions` using the `inspect`
module. It then sets the according attributes with instances of the
AsyncMock class.
There is one caveat: `discord.py` very rarely defines regular methods
that return a coroutine object. Since the returned coroutine should
still be awaited, these regular methods should also be mocked with an
AsyncMock. However, since they are regular methods, `inspect` does
not detect them and they have to be added manually. (The only case of
this I've found so far is `Client.wait_for`.)
2. Properly set special attributes using `kwargs.get`
As we want attributes that point to other discord.py objects to use
our custom mocks (.e.g, `Message.author` should use `MockMember`),
the `__init__` method of our custom mocks make sure to correctly
instantiate these attributes.
However, the way we previously did that means we can't instantiate
the custom mock with a mock instance we provide, since this special
instantiation would overwrite the custom object we'd passed. I've
solved this by using `kwargs.get`, with a new mock as the default
value. This makes sure we only create a new mock if we didn't pass
a custom one:
```py
class MockMesseage:
def __init__(self, **kwargs):
self.author = kwargs.get('author', MockMember())
```
As you can see, we will only create a new MockMember if we did not
pass an `author` argument.
3. Factoring out duplicate lines
Since our `CustomMockMixin` is a parent to all of our custom mock
types, it makes sense to use it to factor out common code of all of
our custom mocks.
I've made the following changes:
- Set a default child mock type in the mixin.
- Create an `__init__` that takes care of the `inspect` of point 1
This means we won't have to repeat this in all of the child classes.
4. Three new Mock types: Emoji, PartialEmoji, and Reaction
I have added three more custom mocks:
- MockEmoji
- MockPartialEmoji
- MockReaction
I accidentally forgot to update the docstring of `CustomMockMixin`, which changed quite dramatically in scope with the last commit. This commit remedies that. In addition, I inadvertently forgot to remove the `child_mock_type` class attribute from `MockRole`. Since it uses the default value, it is no longer necessary to specify it in the child class as well.
lemonsaurus
approved these changes
Oct 31, 2019
Contributor
lemonsaurus
left a comment
There was a problem hiding this comment.
this is smart as hell. nice work.
MarkKoz
approved these changes
Oct 31, 2019
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.
I have made several changes to our test helpers defined in
tests/helpers.py. These changes will make it easier to work with the mock objects as well as fixing a bug. All changes are backwards compatible with all the current tests we have and should be compatible with the tests that are currently being written as well.Better way of controlling child mock types
We previously used an override of the
__new__method to prevent our custom mock types from instantiating their children with their own type instead of a general mock type likeMagicMockorMock.As it turns out, the Python documentation suggests another way of doing this that does not involve overriding
__new__. This commit implements this new method to make sure we're using the idiomatic way of handling this.The suggested method is overriding the
_get_child_mockmethod in the subclass. I've added this override to amixinthat we can apply to all of the custom mock types we define. If you want to use this mixin, please make sure that it comes before the general mock type in the MRO.AsyncMock's call now returns the right mock type
Since mocks normally instantiate children with their own type, our
AsyncMocktype returned a newAsyncMockafter it had been awaited. By using the mixin described in the previous section, it now returns a regularMagicMock.Automatically create AsyncMock attributes using
inspectNormally, attributes accessed on a mock are automatically created with an instance of either Mock or MagicMock. However, since those only support synchronous calls, this is not desirable for coroutine function methods. That's why our
__init__methods previously had a long list of all the coroutines defined for a class to properly mock it with anAsyncMock. However, hard-coding such a list is prone to human error and not resilient to changes introduced by version upgrades.Instead, I have now created a helper method in our
CustomMockMixin(formerlyGetChildMockMixin) that automatically inspects thespecinstance we've passed forcoroutine functionsusing theinspectmodule. It then automatically sets the according attributes with instances of theAsyncMockclass.There is one caveat:
discord.pyvery rarely defines regular methods that return a coroutine object. Since the returned coroutine should still be awaited, these regular methods should also be mocked with anAsyncMock. However, since they are regular methods,inspectdoes not detect them and they have to be added manually. (The only case of this I've found so far isClient.wait_for.)Better handling of passed keyword arguments
Normally, a Mock type respects the keyword arguments passed and automatically sets them as attributes of the mock instance. However, some of custom Mock types set custom attributes, making this impossible:
In this snippet, we attempt to set the
authorattribute to a mock instance withname="Guido", but since__init__sets this attribute after thesuper().__init__, this will be lost. To solve this, I've made sure that we only instantiate a new mock if the attribute name was not present inkwargsby leveraging the default value of.get:Added MockEmoji, MockPartialEmoji, MockReaction
I have added three new custom mock types, MockEmoij, MockPartialEmoji, and MockReaction after a request from @lemonsaurus.