bpo-35047: Better error messages in unittest.mock by PetterS · Pull Request #10090 · python/cpython

PetterS · 2018-10-25T08:02:25Z

When developing unit tests with unittest.mock, it is common to see error messages like this:

AssertionError: Expected 'info' to not have been called. Called 3 times.

This commit adds the actual list of calls to the error message.

https://bugs.python.org/issue35047

PetterS · 2018-10-25T08:06:06Z

@eamanu here is a PR for the issue I filed.

When developing unit tests with `unittest.mock`, it is common to see error messages like this: AssertionError: Expected 'info' to not have been called. Called 3 times. This commit adds the actual list of calls to the error message.

eamanu

it's ok for me.

eamanu · 2018-10-25T13:24:58Z

@PetterS look the travis error

PetterS · 2018-10-25T19:23:34Z

The Travis error is that an extra string is printed in the documentation. That string only appears here: https://github.com/python/cpython/blame/e42b705188271da108de42b55d9344642170aa2b/Doc/library/weakref.rst#L493
It is not clear to me if/why my change has anything to do with that.

PetterS · 2018-10-25T19:42:55Z

@vstinner, you seem to have been involved in several PRs involving mock. Do you think you could take a look if time allows?

vstinner

What about other methods like assert_called_once_with()?

If you use safe_repr(), please modify other exceptions including mock_calls, like assert_has_calls().

cc @mariocj89

bedevere-bot · 2018-10-25T21:16:17Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

mariocj89

Note: The link in the PR description does not match the one in the title.

Also, there are no tests validating this change :)

pablogsal · 2018-10-25T23:28:56Z

Notice some examples in library/unittest.mock-examples.rst need to be modified as well to fix the doctests.

PetterS · 2018-10-27T08:01:04Z

@pablogsal This PR modified assert_called_once and assert_not_called, neither of which appears in library/unittest.mock-examples.rst. So I am not sure which changes to make. Anyway, I'll make some requested changes so perhaps it will become clear

PetterS · 2018-10-27T10:14:19Z

I have made the requested changes; please review again.

bedevere-bot · 2018-10-27T10:14:22Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

bedevere-bot · 2018-10-28T15:58:25Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

- Rename helper function. Add optional argument. - Use re.compile. - Change NEWS entry.

PetterS · 2018-10-28T18:25:17Z

Thanks for the review! All valid points. I have made the requested changes; please review again.

bedevere-bot · 2018-10-28T18:25:19Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner

LGTM.

vstinner · 2018-10-28T18:32:21Z

@mariocj89: Would you mind to double check the PR, please?

mariocj89 · 2018-10-28T18:43:39Z

@@ -0,0 +1,3 @@
+``unittest.mock`` now includes mock calls in exception messages if


Nitpick: the change affects also assert_called_once_with and assert_has_calls

assert_has_calls was only refactored. Is it OK to write assert_called_once(_with)?

Good point!

About assert_called_once_with, I'd just list the three

mariocj89

LGTM. Thanks for the patience going through the review :)

vstinner · 2018-10-28T20:41:47Z

Thanks for the patience going through the review :)

I concur :-) But the final change is much better than the first version, so it was worth it ;-)

If some people complain that the truncated "Calls: ..." is too short, maybe we can discuss the length later. But right now, let's see how users like safe_repr() default length ;-)

PetterS · 2018-10-28T22:02:12Z

Thanks everyone! I am very pleased with the process of contributing to CPython. 👍

the-knights-who-say-ni added the CLA signed label Oct 25, 2018

bedevere-bot added the awaiting review label Oct 25, 2018

PetterS changed the title ~~bpo-34547: Better error messages in unittest.mock~~ bpo-35047: Better error messages in unittest.mock Oct 25, 2018

bpo-35047: Better error messages in unittest.mock

9b25872

When developing unit tests with `unittest.mock`, it is common to see error messages like this: AssertionError: Expected 'info' to not have been called. Called 3 times. This commit adds the actual list of calls to the error message.

PetterS force-pushed the bpo-35047 branch from 3dac117 to 9b25872 Compare October 25, 2018 08:10

eamanu approved these changes Oct 25, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Oct 25, 2018

vstinner requested changes Oct 25, 2018

View reviewed changes

Comment thread Lib/unittest/mock.py Outdated

Comment thread Lib/unittest/mock.py Outdated

bedevere-bot added awaiting changes and removed awaiting core review labels Oct 25, 2018

mariocj89 reviewed Oct 25, 2018

View reviewed changes

Comment thread Lib/unittest/mock.py Outdated

mariocj89 reviewed Oct 25, 2018

View reviewed changes

Comment thread Lib/unittest/mock.py Outdated

bpo-35047: Add helper method and unit tests.

964e055

mariocj89 reviewed Oct 27, 2018

View reviewed changes

Comment thread Lib/unittest/test/testmock/testmock.py Outdated

PetterS added 2 commits October 27, 2018 11:41

bpo-35047: Run make patchcheck.

8e1ad76

bpo-35047: Use assertNotIn.

a66130f

bedevere-bot added awaiting change review and removed awaiting changes labels Oct 27, 2018

vstinner requested changes Oct 28, 2018

View reviewed changes

Comment thread Lib/unittest/mock.py Outdated

Comment thread Lib/unittest/mock.py Outdated

Comment thread Lib/unittest/test/testmock/testmock.py Outdated

Comment thread Misc/NEWS.d/next/Library/2018-10-25-09-59-00.bpo-35047.abbaa.rst Outdated

bedevere-bot added awaiting changes and removed awaiting change review labels Oct 28, 2018

bpo-35047: Changes per review.

fb871de

- Rename helper function. Add optional argument. - Use re.compile. - Change NEWS entry.

bedevere-bot removed the awaiting changes label Oct 28, 2018

bedevere-bot added the awaiting change review label Oct 28, 2018

vstinner approved these changes Oct 28, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Oct 28, 2018

mariocj89 approved these changes Oct 28, 2018

View reviewed changes

bpo-35047: List all three functions in the NEWS entry.

a349374

mariocj89 approved these changes Oct 28, 2018

View reviewed changes

vstinner merged commit 47d9424 into python:master Oct 28, 2018

bedevere-bot removed the awaiting merge label Oct 28, 2018

		@@ -0,0 +1,3 @@
		``unittest.mock`` now includes mock calls in exception messages if

Uh oh!

Uh oh!

Conversation

PetterS commented Oct 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PetterS commented Oct 25, 2018

Uh oh!

eamanu left a comment

Choose a reason for hiding this comment

Uh oh!

eamanu commented Oct 25, 2018

Uh oh!

PetterS commented Oct 25, 2018

Uh oh!

PetterS commented Oct 25, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Oct 25, 2018

Uh oh!

mariocj89 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pablogsal commented Oct 25, 2018

Uh oh!

PetterS commented Oct 27, 2018

Uh oh!

Uh oh!

PetterS commented Oct 27, 2018

Uh oh!

bedevere-bot commented Oct 27, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Oct 28, 2018

Uh oh!

PetterS commented Oct 28, 2018

Uh oh!

bedevere-bot commented Oct 28, 2018

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 28, 2018

Uh oh!

mariocj89 Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

PetterS Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

mariocj89 Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

PetterS Oct 28, 2018

Choose a reason for hiding this comment

Uh oh!

mariocj89 left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 28, 2018

Uh oh!

PetterS commented Oct 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

PetterS commented Oct 25, 2018 •

edited

Loading

mariocj89 left a comment •

edited

Loading