From c3e7312f3e023bd55df50c05eeee6423845d272b Mon Sep 17 00:00:00 2001 From: Samuel Freilich Date: Wed, 11 Sep 2019 15:22:54 -0400 Subject: [PATCH 1/6] bpo-36871: Handle spec errors in assert_has_calls The fix in PR 13261 handled the underlying issue about the spec for specific methods not being applied correctly, but it didn't fix the issue that was causing the misleading error message. The code currently grabs a list of responses from _call_matcher (which may include exceptions). But it doesn't reach inside the list when checking if the result is an exception. This results in a misleading error message when one of the provided calls does not match the spec. --- Lib/unittest/mock.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index a98decc2c9f008..cd18d314c9223e 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -2214,7 +2214,7 @@ def assert_has_awaits(self, calls, any_order=False): they must all appear in :attr:`await_args_list`. """ expected = [self._call_matcher(c) for c in calls] - cause = expected if isinstance(expected, Exception) else None + cause = next((e for e in expected if isinstance(e, Exception)), None) all_awaits = _CallList(self._call_matcher(c) for c in self.await_args_list) if not any_order: if expected not in all_awaits: From 6e44f0c545ba453ba008c982cf53b86482b49261 Mon Sep 17 00:00:00 2001 From: Samuel Freilich Date: Wed, 11 Sep 2019 21:29:44 -0400 Subject: [PATCH 2/6] Fix error-handling bug in assert_has_calls As well as assert_has_awaits, where I misplaced that fix previously. Add tests for both fixes. --- Lib/unittest/mock.py | 2 +- Lib/unittest/test/testmock/testasync.py | 8 ++++++++ Lib/unittest/test/testmock/testmock.py | 7 +++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index cd18d314c9223e..6b900d70d7e365 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -931,7 +931,7 @@ def assert_has_calls(self, calls, any_order=False): If `any_order` is True then the calls can be in any order, but they must all appear in `mock_calls`.""" expected = [self._call_matcher(c) for c in calls] - cause = expected if isinstance(expected, Exception) else None + cause = next((e for e in expected if isinstance(e, Exception)), None) all_calls = _CallList(self._call_matcher(c) for c in self.mock_calls) if not any_order: if expected not in all_calls: diff --git a/Lib/unittest/test/testmock/testasync.py b/Lib/unittest/test/testmock/testasync.py index 550234223cc372..0db6165da2b6c9 100644 --- a/Lib/unittest/test/testmock/testasync.py +++ b/Lib/unittest/test/testmock/testasync.py @@ -632,3 +632,11 @@ def test_assert_not_awaited(self): asyncio.run(self._runnable_test()) with self.assertRaises(AssertionError): self.mock.assert_not_awaited() + + def test_assert_has_awaits_not_matching_spec_error(self): + async def f(): pass + + mock = AsyncMock(spec=f) + with self.assertRaises(AssertionError) as cm: + mock.assert_has_awaits([call('wrong')]) + self.assertIsInstance(cm.exception.__cause__, TypeError) diff --git a/Lib/unittest/test/testmock/testmock.py b/Lib/unittest/test/testmock/testmock.py index 581afaaeb815df..d2d5a71a501a34 100644 --- a/Lib/unittest/test/testmock/testmock.py +++ b/Lib/unittest/test/testmock/testmock.py @@ -1426,6 +1426,13 @@ def f(a, b, c, d=None): pass mock.assert_has_calls(calls[:-1]) mock.assert_has_calls(calls[:-1], any_order=True) + def test_assert_has_calls_not_matching_spec_error(self): + def f(): pass + + mock = Mock(spec=f) + with self.assertRaises(AssertionError) as cm: + mock.assert_has_calls([call('wrong')]) + self.assertIsInstance(cm.exception.__cause__, TypeError) def test_assert_any_call(self): mock = Mock() From f0ceae7cee8838167c51a80e429f9bb9f8ffc7b2 Mon Sep 17 00:00:00 2001 From: Samuel Freilich Date: Thu, 12 Sep 2019 11:17:11 -0400 Subject: [PATCH 3/6] Include all exceptions in error message Include all exceptions deferred when binding expected calls with the spec in the error messages generated by assert_has_calls and assert_has_awaits. To make the origin of each exception clear, the exceptions are presented in a list parallel to the expected calls, with None at positions where processing the call generated no exception. The first exception encountered is still recorded as the cause of the AssertionError, since that's the exception which would have been raised had the handling of those exceptions not been deferred. --- Lib/unittest/mock.py | 22 +++++++++++++++++++--- Lib/unittest/test/testmock/testasync.py | 17 +++++++++++++++-- Lib/unittest/test/testmock/testmock.py | 16 ++++++++++++++-- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index 6b900d70d7e365..424a6190069ebc 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -935,9 +935,17 @@ def assert_has_calls(self, calls, any_order=False): all_calls = _CallList(self._call_matcher(c) for c in self.mock_calls) if not any_order: if expected not in all_calls: + if cause is None: + problem = 'Calls not found.' + else: + problem = ('Error processing expected calls.\n' + 'Errors: {}').format( + [e if isinstance(e, Exception) else None + for e in expected]) raise AssertionError( - 'Calls not found.\nExpected: %r%s' - % (_CallList(calls), self._calls_repr(prefix="Actual")) + f'{problem}\n' + f'Expected: {_CallList(calls)}\n' + f'Actual: {self._calls_repr(prefix="Actual")}' ) from cause return @@ -2218,8 +2226,16 @@ def assert_has_awaits(self, calls, any_order=False): all_awaits = _CallList(self._call_matcher(c) for c in self.await_args_list) if not any_order: if expected not in all_awaits: + if cause is None: + problem = 'Awaits not found.' + else: + problem = ('Error processing expected awaits.\n' + 'Errors: {}').format( + [e if isinstance(e, Exception) else None + for e in expected]) raise AssertionError( - f'Awaits not found.\nExpected: {_CallList(calls)}\n' + f'{problem}\n' + f'Expected: {_CallList(calls)}\n' f'Actual: {self.await_args_list}' ) from cause return diff --git a/Lib/unittest/test/testmock/testasync.py b/Lib/unittest/test/testmock/testasync.py index 0db6165da2b6c9..6196804366829b 100644 --- a/Lib/unittest/test/testmock/testasync.py +++ b/Lib/unittest/test/testmock/testasync.py @@ -1,5 +1,6 @@ import asyncio import inspect +import re import unittest from unittest.mock import (call, AsyncMock, patch, MagicMock, create_autospec, @@ -637,6 +638,18 @@ def test_assert_has_awaits_not_matching_spec_error(self): async def f(): pass mock = AsyncMock(spec=f) - with self.assertRaises(AssertionError) as cm: - mock.assert_has_awaits([call('wrong')]) + + with self.assertRaisesRegex( + AssertionError, + re.escape('Awaits not found.\nExpected:')) as cm: + mock.assert_has_awaits([call()]) + self.assertIsNone(cm.exception.__cause__) + + with self.assertRaisesRegex( + AssertionError, + re.escape('Error processing expected awaits.\n' + "Errors: [None, TypeError('too many positional " + "arguments')]\n" + 'Expected:')) as cm: + mock.assert_has_awaits([call(), call('wrong')]) self.assertIsInstance(cm.exception.__cause__, TypeError) diff --git a/Lib/unittest/test/testmock/testmock.py b/Lib/unittest/test/testmock/testmock.py index d2d5a71a501a34..ad464af3d31cbe 100644 --- a/Lib/unittest/test/testmock/testmock.py +++ b/Lib/unittest/test/testmock/testmock.py @@ -1430,8 +1430,20 @@ def test_assert_has_calls_not_matching_spec_error(self): def f(): pass mock = Mock(spec=f) - with self.assertRaises(AssertionError) as cm: - mock.assert_has_calls([call('wrong')]) + + with self.assertRaisesRegex( + AssertionError, + re.escape('Calls not found.\nExpected:')) as cm: + mock.assert_has_calls([call()]) + self.assertIsNone(cm.exception.__cause__) + + with self.assertRaisesRegex( + AssertionError, + re.escape('Error processing expected calls.\n' + "Errors: [None, TypeError('too many positional " + "arguments')]\n" + 'Expected:')) as cm: + mock.assert_has_calls([call(), call('wrong')]) self.assertIsInstance(cm.exception.__cause__, TypeError) def test_assert_any_call(self): From 587eb5512fdc29ac751de8f895a2fb07329b9f08 Mon Sep 17 00:00:00 2001 From: Samuel Freilich Date: Thu, 12 Sep 2019 12:53:38 -0400 Subject: [PATCH 4/6] Fix whitespace lint --- Lib/unittest/mock.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index 424a6190069ebc..9dd87fb5397187 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -936,12 +936,12 @@ def assert_has_calls(self, calls, any_order=False): if not any_order: if expected not in all_calls: if cause is None: - problem = 'Calls not found.' + problem = 'Calls not found.' else: - problem = ('Error processing expected calls.\n' - 'Errors: {}').format( - [e if isinstance(e, Exception) else None - for e in expected]) + problem = ('Error processing expected calls.\n' + 'Errors: {}').format( + [e if isinstance(e, Exception) else None + for e in expected]) raise AssertionError( f'{problem}\n' f'Expected: {_CallList(calls)}\n' @@ -2227,12 +2227,12 @@ def assert_has_awaits(self, calls, any_order=False): if not any_order: if expected not in all_awaits: if cause is None: - problem = 'Awaits not found.' + problem = 'Awaits not found.' else: - problem = ('Error processing expected awaits.\n' - 'Errors: {}').format( - [e if isinstance(e, Exception) else None - for e in expected]) + problem = ('Error processing expected awaits.\n' + 'Errors: {}').format( + [e if isinstance(e, Exception) else None + for e in expected]) raise AssertionError( f'{problem}\n' f'Expected: {_CallList(calls)}\n' From 95beaaa2a9b94c38d31709076001327ee5365ac3 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Tue, 24 Sep 2019 18:45:47 +0000 Subject: [PATCH 5/6] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst b/Misc/NEWS.d/next/Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst new file mode 100644 index 00000000000000..34f55172943f2f --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst @@ -0,0 +1 @@ +Improve error handling for the assert_has_calls and assert_has_awaits methods of mocks. Fixed a bug where any errors encountered while binding the expected calls to the mock's spec were silently swallowed, leading to misleading error output. \ No newline at end of file From 4e9bd60dbd7ff38c5b41776173ffac4b5bbfdadc Mon Sep 17 00:00:00 2001 From: Samuel Freilich Date: Tue, 24 Sep 2019 14:48:50 -0400 Subject: [PATCH 6/6] Line breaks (Thought the blurb-it tool would handle that.) --- .../2019-09-24-18-45-46.bpo-36871.p47knk.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst b/Misc/NEWS.d/next/Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst index 34f55172943f2f..6b7b19a0d57ee6 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2019-09-24-18-45-46.bpo-36871.p47knk.rst @@ -1 +1,3 @@ -Improve error handling for the assert_has_calls and assert_has_awaits methods of mocks. Fixed a bug where any errors encountered while binding the expected calls to the mock's spec were silently swallowed, leading to misleading error output. \ No newline at end of file +Improve error handling for the assert_has_calls and assert_has_awaits methods of +mocks. Fixed a bug where any errors encountered while binding the expected calls +to the mock's spec were silently swallowed, leading to misleading error output.