HACKING.rst: more unit testing documentation

[OddBloke]

Specifically:

• include warning against general use of mock assert methods (and suggestions on how to replace them)
• add guidelines on test decorator/param ordering
• add test guideline for module-level mock variables

[lucasmoura]

I really like all of the proposed ideas. Maybe we can extend the mock syntax replacement to other projects as well. For example, curtin also has those mock test and maybe this would be a useful guideline there too

[TheRealFalcon]

Potential point of discussion

[TheRealFalcon]

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?

[OddBloke]

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)

[TheRealFalcon]

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.