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

bpo-35500: align expected and actual calls on mock.assert_called_with error message. #11804

Merged
merged 7 commits into from Feb 14, 2019

Conversation

Projects
None yet
6 participants
@suhearsawho
Copy link
Contributor

suhearsawho commented Feb 9, 2019

This patch adds an enhancement to the unittest module by adding a newline between the Assertion Error and Expected fields of the error messages generated by mock.assert_called_with. With this addition, the output is clearer as it is easier to read.

https://bugs.python.org/issue35500

@the-knights-who-say-ni

This comment has been minimized.

Copy link

the-knights-who-say-ni commented Feb 9, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

taleinat and others added some commits Feb 10, 2019

Update Misc/NEWS.d/next/Library/2019-02-10-00-00-13.bpo-35500.1HOMmo.rst
Co-Authored-By: suhearsawho <susansu.software@gmail.com>
@taleinat
Copy link
Contributor

taleinat left a comment

This looks good to me, but I'm still waiting for the issue creator to respond to the suggestions to replace the word "actual" with something longer to achieve vertical alignment between the "Expected" and "Actual" lines.

@tirkarthi

This comment has been minimized.

Copy link
Contributor

tirkarthi commented Feb 10, 2019

@taleinat @suhearsawho I just responded here : https://bugs.python.org/issue35500#msg335143. I don't really have a strong opinion on the wording and it's a matter of personal preference of mine on using "Expected" and "Actual". It would be a good improvement with the error message on separate lines itself. So I would be happy to see this landing even with a different wording.

@tirkarthi
Copy link
Contributor

tirkarthi left a comment

LGTM as per https://bugs.python.org/issue35500#msg335145. Please sign the CLA.

Minor nit : You might want to use the below format for hyperlinking to assert_called_with method docs.

Write expected and actual call parameters on separate lines in :meth:`unittest.mock.Mock.assert_called_with` assertion errors.  Contributed by Susan Su.

Thanks @suhearsawho for your contribution.

Update 2019-02-10-00-00-13.bpo-35500.1HOMmo.rst
Changed format for hyperlinking to assert_called_with method (Suggestion from @tirkarthi).
@taleinat
Copy link
Contributor

taleinat left a comment

LGTM. Just waiting for approval of CLA signing.

@suhearsawho

This comment has been minimized.

Copy link
Contributor Author

suhearsawho commented Feb 11, 2019

CLA has been approved.

suhearsawho added some commits Feb 13, 2019

bpo-35500: align expected and actual calls on mock.assert_called_with…
… when self.call_args is None. This commit aims to create consistency with other case in mock.assert_called_with
@suhearsawho

This comment has been minimized.

Copy link
Contributor Author

suhearsawho commented Feb 13, 2019

Updated the code for the case when self.call_args is None. I declared an actual variable in the mock.py file and testmock.py file to maintain consistency with the rest of the code. Please let me know if there is anything I should revise!

@lisroach

This comment has been minimized.

Copy link
Contributor

lisroach commented Feb 14, 2019

Looks great! Thank you @suhearsawho for your first contribution 😃

@lisroach lisroach merged commit 2bdd585 into python:master Feb 14, 2019

5 checks passed

Azure Pipelines PR #20190213.29 succeeded
Details
bedevere/issue-number Issue number 35500 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment