Skip to content

HACKING.rst: more unit testing documentation #354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
OddBloke merged 4 commits into from
May 11, 2020
+55 −0
Merged

HACKING.rst: more unit testing documentation #354

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a253d4b
HACKING.rst: include warning against mock assert methods
OddBloke May 6, 2020
64f2785
HACKING.rst: add guidelines on test decorator/param ordering
OddBloke May 6, 2020
20f743d
HACKING.rst: add test guideline for module-level mock variables
OddBloke May 6, 2020
d13f470
HACKING.rst: change assert_ method docs to permit use with autospeccing
OddBloke May 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions HACKING.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,12 +214,67 @@ The following guidelines should be followed:

* For example, ``m_readurl`` (which would be a mock for ``readurl``)

* The ``assert_*`` methods that are available on ``Mock`` and
``MagicMock`` objects should be avoided, as typos in these method
names may not raise ``AttributeError`` (and so can cause tests to
silently pass). An important exception: if a ``Mock`` is
`autospecced`_ then misspelled assertion methods *will* raise an
``AttributeError``, so these assertion methods may be used on
autospecced ``Mock`` objects.

For non-autospecced ``Mock`` s, these substitutions can be used
(``m`` is assumed to be a ``Mock``):

* ``m.assert_any_call(*args, **kwargs)`` => ``assert
mock.call(*args, **kwargs) in m.call_args_list``
* ``m.assert_called()`` => ``assert 0 != m.call_count``
* ``m.assert_called_once()`` => ``assert 1 == m.call_count``
* ``m.assert_called_once_with(*args, **kwargs)`` => ``assert
[mock.call(*args, **kwargs)] == m.call_args_list``
* ``m.assert_called_with(*args, **kwargs)`` => ``assert
mock.call(*args, **kwargs) == m.call_args_list[-1]``
* ``m.assert_has_calls(call_list, any_order=True)`` => ``for call in
call_list: assert call in m.call_args_list``

* ``m.assert_has_calls(...)`` and ``m.assert_has_calls(...,
any_order=False)`` are not easily replicated in a single
statement, so their use when appropriate is acceptable.

* ``m.assert_not_called()`` => ``assert 0 == m.call_count``
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon May 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

pylint and most editors should highlight methods not found. If we require linting on our commits, I don't think we should restrict these methods.

Python will raise an exception if you create a mock method that starts with "assert" or "assret" that isn't one of the defined ones, so I don't think there's that much risk here. https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L634

I've used these methods and personally find them a lot clearer than the naked asserts. Could just be bias for what I'm used to though.

Should we favor autospeccing instead?

Copy link
Copy Markdown
Collaborator Author

@OddBloke OddBloke May 11, 2020

Choose a reason for hiding this comment

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

Thanks for the feedback James!

Python will raise an exception if you create a mock method that starts with "assert" or "assret" that isn't one of the defined ones, so I don't think there's that much risk here. https://github.com/python/cpython/blob/master/Lib/unittest/mock.py#L634

I agree that there isn't a huge amount of risk here, but the fact that changes have been made to Python to surface this issue to users indicates, to me, that's it's a bigger problem than not. It identifies that there is enough of a problem to change unittest.mocks behaviour, but the change to the library doesn't actually protect against all typos that could be made in these methods.

(And, to be clear, I don't think there's any way that you can protect against all typos. The assertions should not be in the mock object's namespace: that's the fundamental design mistake in these methods, IMO.)

I've used these methods and personally find them a lot clearer than the naked asserts.

That's an important point. For me, I've never used these methods so I simply see them as a code smell. But they do make test code easier to understand for folks who are used to them, and that's valuable.

Should we favor autospeccing instead?

Perhaps we could change this text to say that we allow the usage of these methods but only on mocks that have a spec/are autospec'd? Does that sound reasonable? (We should also be doing more autospec'ing in general, no doubt.)

I think that the "permissible if autospec'd" is probably our best path forward but, for completeness, an alternative that does allow us to catch all typos would be to lookup the methods on the Mock type and then call them with our mock:

m = Mock()
m.asert_called_once()  # Will silently pass
Mock.asert_called_once(m)  # Will fail with AttributeError

If we really wanted, we could set these up as helper functions (which has been proposed for CPython) somewhere:

# helpers.py
from unittest.mock import Mock

assert_called_once = Mock.assert_called_once

# test code
from ...helpers import assert_called_once

m = Mock()
assert_called_once(m)

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon May 11, 2020

Choose a reason for hiding this comment

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

Perhaps we could change this text to say that we allow the usage of these methods but only on mocks that have a spec/are autospec'd? Does that sound reasonable? (We should also be doing more autospec'ing in general, no doubt.)

That seems reasonable. It would also encourage me to actually use autospecing as I usually just default to a standard mock anyway.

I think I currently like the autospecing exception without the additional helper functions. If we decide we want them later, we can always add them.


* Test arguments should be ordered as follows:

* ``mock.patch`` arguments. When used as a decorator, ``mock.patch``
partially applies its generated ``Mock`` object as the first
argument, so these arguments must go first.
* ``pytest.mark.parametrize`` arguments, in the order specified to
the ``parametrize`` decorator. These arguments are also provided
by a decorator, so it's natural that they sit next to the
``mock.patch`` arguments.
* Fixture arguments, alphabetically. These are not provided by a
decorator, so they are last, and their order has no defined
meaning, so we default to alphabetical.

* It follows from this ordering of test arguments (so that we retain
the property that arguments left-to-right correspond to decorators
bottom-to-top) that test decorators should be ordered as follows:

* ``pytest.mark.parametrize``
* ``mock.patch``

* When there are multiple patch calls in a test file for the module it
is testing, it may be desirable to capture the shared string prefix
for these patch calls in a module-level variable. If used, such
variables should be named ``M_PATH`` or, for datasource tests,
``DS_PATH``.

.. _pytest: https://docs.pytest.org/
.. _pytest fixtures: https://docs.pytest.org/en/latest/fixture.html
.. _TestGetPackageMirrorInfo: https://github.com/canonical/cloud-init/blob/42f69f410ab8850c02b1f53dd67c132aa8ef64f5/cloudinit/distros/tests/test_init.py\#L15
.. _TestPrependBaseCommands: https://github.com/canonical/cloud-init/blob/master/cloudinit/tests/test_subp.py#L9
.. _assertion introspection: https://docs.pytest.org/en/latest/assert.html
.. _pytest 3.0: https://docs.pytest.org/en/latest/changelog.html#id1093
.. _autospecced: https://docs.python.org/3.8/library/unittest.mock.html#autospeccing

Type Annotations
----------------
Expand Down