Skip to content
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.

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

Align expected and actual calls on mock.assert_called_with error message #79681

Closed
tirkarthi opened this issue Dec 14, 2018 · 16 comments
Closed
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@tirkarthi
Copy link
Member

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
    Copy link
    Member Author

    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

    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)].

    @tirkarthi tirkarthi added 3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Dec 14, 2018
    @mariocj89
    Copy link
    Mannequin

    mariocj89 mannequin commented Dec 15, 2018

    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
    Copy link
    Member Author

    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
    Copy link
    Member

    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
    Copy link
    Member Author

    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.

    @AdnanUmer AdnanUmer mannequin removed the 3.8 only security fixes label Dec 24, 2018
    @taleinat
    Copy link
    Contributor

    taleinat commented Jan 2, 2019

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

    @terryjreedy
    Copy link
    Member

    "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'.

    @terryjreedy terryjreedy added the 3.8 only security fixes label Jan 2, 2019
    @csabella
    Copy link
    Contributor

    @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
    Copy link
    Member Author

    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
    Copy link
    Contributor

    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
    Copy link
    Member Author

    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
    Copy link
    Contributor

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

    @suhearsawho
    Copy link
    Mannequin

    suhearsawho mannequin commented Feb 13, 2019

    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
    Copy link
    Contributor

    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
    Copy link
    Contributor

    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
    Copy link
    Contributor

    Thanks everyone for seeing this through!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants