bpo-37828: Fix default mock_name in unittest.mock.assert_called error #16166
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA). CLA MissingOur records indicate the following people have not signed the CLA: For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. If you have recently signed the CLA, please wait at least one business day You can check yourself to see if the CLA has been received. Thanks again for the contribution, we look forward to reviewing it! |
There was a problem hiding this comment.
Quite a nice fix @categulario!
Would be great if you can add a test to make sure this does not happen again. You can do so by adding a test like
def test_assert_called_exception_message():
with self.assertRaisesRegexp(AssertionError, "Expected 'mock' to have been called"):
Mock().assert_called()
with self.assertRaisesRegexp(AssertionError, "Expected 'test_name' to have been called"):
Mock(name="test_name").assert_called()Also don't forget to go through the process for the CLA so the contribution can be accepted.
You should also include a news entry, see here.
This patch should be probably backported as well (for a core dev to mark as such).
* add missing `self` parameter to test case * use `assertRaisesRegex` instead of `assertRaisesRegexp`
|
Thanks for the review @mariocj89!
|
Misc/NEWS.d/next/Library/2019-09-15-21-31-18.bpo-37828.gLLDX7.rst
Outdated
Show resolved
Hide resolved
| @@ -396,6 +396,13 @@ def _check(mock): | |||
| _check(mock) | |||
|
|
|||
|
|
|||
| def test_assert_called_exception_message(self): | |||
| with self.assertRaisesRegex(AssertionError, "Expected 'mock' to have been called"): | |||
There was a problem hiding this comment.
Nit : Please follow PEP 8 guidelines to ensure the line length is not more than 80 characters here and in below. You can probably have it as a format string with message and fill in the mock name like below.
message = "Expected '{0}' to have been called"
with self.assertRaisesRegex(AssertionError, message.format('mock')):
Mock().assert_called()There was a problem hiding this comment.
Thanks! What is the preferred way to run pep8 agains the modified code?
There was a problem hiding this comment.
I only do manual checking. There are some parts of the code that predates PEP8 and PEP8 is enforced only for new code so using pep8 libraries on whole file might give some false information.
* A more descriptive blurb message
* Fix code to follow pep8
|
ping @pablogsal |
|
Thanks @categulario for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
|
I'm having trouble backporting to |
|
Thanks @categulario for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
…pythonGH-16166) In the format string for assert_called the evaluation order is incorrect and hence for mock's without name, 'None' is printed whereas it should be 'mock' like for other messages. The error message is ("Expected '%s' to have been called." % self._mock_name or 'mock'). (cherry picked from commit 5f5f11f) Co-authored-by: Abraham Toriz Cruz <awonderfulcode@gmail.com>
|
GH-16229 is a backport of this pull request to the 3.8 branch. |
…GH-16166) In the format string for assert_called the evaluation order is incorrect and hence for mock's without name, 'None' is printed whereas it should be 'mock' like for other messages. The error message is ("Expected '%s' to have been called." % self._mock_name or 'mock'). (cherry picked from commit 5f5f11f) Co-authored-by: Abraham Toriz Cruz <awonderfulcode@gmail.com>
I used parenthesis to change precedence so the correct message is displayed. This is my very fist contribution to python (:
https://bugs.python.org/issue37828