Align expected and actual calls on mock.assert_called_with error message

[tirkarthi]

BPO	35500
Nosy	@terryjreedy, @taleinat, @cjw296, @voidspace, @lisroach, @csabella, @mariocj89, @tirkarthi, @suhearsawho
PRs	bpo-35500: align expected and actual calls on mock.assert_called_with error message. #11804

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2019-02-17.07:59:22.581>
created_at = <Date 2018-12-14.18:45:46.316>
labels = ['3.8', 'type-feature', 'library']
title = 'Align expected and actual calls on mock.assert_called_with error message'
updated_at = <Date 2019-02-17.07:59:22.581>
user = 'https://github.com/tirkarthi'

bugs.python.org fields:

activity = <Date 2019-02-17.07:59:22.581>
actor = 'taleinat'
assignee = 'none'
closed = True
closed_date = <Date 2019-02-17.07:59:22.581>
closer = 'taleinat'
components = ['Library (Lib)']
creation = <Date 2018-12-14.18:45:46.316>
creator = 'xtreak'
dependencies = []
files = []
hgrepos = []
issue_num = 35500
keywords = ['patch', 'patch', 'patch', 'patch']
message_count = 16.0
messages = ['331849', '331909', '331916', '332302', '332303', '332893', '332900', '334516', '334521', '335142', '335143', '335145', '335391', '335394', '335491', '335759']
nosy_count = 10.0
nosy_names = ['terry.reedy', 'taleinat', 'cjw296', 'michael.foord', 'lisroach', 'cheryl.sabella', 'mariocj89', 'xtreak', 'Adnan Umer', 'suhearsawho']
pr_nums = ['11804']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue35500'
versions = ['Python 3.8']

[tirkarthi]

Currently, assert_called_with has expected calls list in the same line with AssertionError that causes the visualizing the difference to be hard. It will be great if Expected call occurs on the next line so that the diff is improved. The change has to be made at

cpython/Lib/unittest/mock.py

Line 749 in f8e9bd5

message = 'Expected call: %s\nActual call: %s'

.

from unittest import mock

m = mock.Mock()
m(1, 2)
m.assert_called_with(2, 3)

Current output :

Traceback (most recent call last):
  File "/tmp/bar.py", line 5, in <module>
    m.assert_called_with(2, 3)
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/mock.py", line 820, in assert_called_with
    raise AssertionError(_error_message()) from cause
AssertionError: Expected call: mock(2, 3)
Actual call: mock(1, 2)

Proposed output :

Traceback (most recent call last):
  File "/tmp/bar.py", line 5, in <module>
    m.assert_called_with(2, 3)
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 827, in assert_called_with
    raise AssertionError(_error_message()) from cause
AssertionError:
Expected call: mock(2, 3)
Actual call: mock(1, 2)

Some more alignment with the call list starting in the same column

AssertionError:
Expected call: mock(2, 3)
Actual call: mock(1, 2)

Originally reported in the GitHub repo at testing-cabal/mock#424 . PR for this was closed since GitHub is used only for backporting (testing-cabal/mock#425). I thought to report it here for discussion. Currently call list output is as per proposed output.

AssertionError: Calls not found.
Expected: [call(1, 2, 3)]
Actual: [call(1, 2)].

[mariocj89]

Makes sense!

I'd not align them though but that might be my view as I generally don't like aligning text like that.

Also if you feel that the exceptions read "weird" with the first sentence is empty, an option might be to say the calls don't match, to make it symmetric with the assert_calls message. Example:

Traceback (most recent call last):
  File "/tmp/bar.py", line 5, in <module>
    m.assert_called_with(2, 3)
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 827, in assert_called_with
    raise AssertionError(_error_message()) from cause
AssertionError: Expected call not found.
Expected call: mock(2, 3)
Actual call: mock(1, 2)

This way all error reports give you the issue in the first line of the message and further details in the next lines.

[tirkarthi]

Thanks Mario for the feedback. The alignment was just a personal preference of mine. I agree with you on adding "Expected call not found" next to AssertionError. I will wait if others have more feedback on this before proceeding on the PR.

[terryjreedy]

We don't usually wrap error messages, but this is plausible. With the addition to the first line, we don't need to repeat 'call'. We can line things up without violating the PEP-8 recommendation against doing so with spaces. Also, the convention is to not capitolize the phrase after ':'.

AssertionError: expected call not found.
Expect: mock(2, 3)
Actual: mock(1, 2)

[tirkarthi]

Thanks for the feedback. I personally prefer 'expected' than 'expect' though it comes at the cost that this cannot be aligned with 'actual'. Other places use 'expected' and it reads more natural in test case scenarios.

[taleinat]

Perhaps "expected" and "observed" or "detected"?

[terryjreedy]

"Expected" and "Observed" seem good. I like "Received" slightly better, but would not argue with PR author. It depends on whether one anthropomorphizes the assert function (or test machinery) as saying 'I expected to see' or 'I expected to get'.

[csabella]

@XTreak, were you going to submit a pull request for this or do you think this would be a 'good first issue' for a new contributor?

[tirkarthi]

Cheryl, this is a good first issue for new contributor. I was waiting for suggestion and I am not sure if there is a consensus over the format. I would like to go with the below format as per suggestion from Mario and Terry.

AssertionError: expected call not found.
Expected: mock(2, 3)
Actual: mock(1, 2)

test_assert_called_with_failure_message has to be fixed as a result of this change. Feel free to mark it as an easy issue.

Thanks

[taleinat]

Karthikeyan, what do you think of the suggestions by Terry and myself to achieve alignment by replacing the word "Actual" with a longer alternative, such as "Observed" or "Received"?

[tirkarthi]

I think "Actual" is more consistent with other error messages like assert_has_calls. I haven't seen usage of observed or received in error messages and hence my opinion on using expected and actual though they are not visually aligned. I don't have a strong opinion on this. I will leave it to the maintainer who wants to merge this change and I am fine as long as the calls are represented on separate lines which is a good improvement over current format.

>>> m.assert_has_calls(mock.call(1))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/karthikeyansingaravelan/stuff/python/cpython/Lib/unittest/mock.py", line 857, in assert_has_calls
    raise AssertionError(
AssertionError: Calls not found.
Expected: ['', (1,), {}]
Actual: [call(1), call(2)].

[taleinat]

After more thought, I agree that it isn't worth changing the current wording from "Actual" to something else.

[suhearsawho]

After taking a look at the assert_called_with function, I noticed that the formatting is inconsistent for the following case:
from unittest import mock

m = mock.Mock()
a = [1, 2, 3, 4]
m.assert_called_with(*a)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/vagrant/cpython/Lib/unittest/mock.py", line 817, in assert_called_with
    raise AssertionError('Expected call: %s\nNot called' % (expected,))
AssertionError: Expected call: mock(1, 2, 3)
Not called

If you believe it would be appropriate, I would like to change the format of the code above to the following:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/vagrant/cpython/Lib/unittest/mock.py", line 817, in assert_called_with
    raise AssertionError(
AssertionError: expected call not found.
Expected: mock(1, 2, 3, 4)
Not called

This way, we would create consistency in our output.

[taleinat]

Susan, I agree that similarly improving the failure message for assert_called_with would be good.

I find the final "Not called" line unclear, though. Perhaps something like the following:

AssertionError: expected call not found.
Expected: mock(1, 2, 3, 4)
Actual: Not called

[lisroach]

New changeset 2bdd585 by Lisa Roach (Susan Su) in branch 'master':
bpo-35500: align expected and actual calls on mock.assert_called_with error message. (GH-11804)
2bdd585

[taleinat]

Thanks everyone for seeing this through!