From d9a5bb7e6acc9a824c5f679fcb3b1c9128acb876 Mon Sep 17 00:00:00 2001 From: Matthew Suozzo Date: Fri, 9 Apr 2021 21:52:41 -0400 Subject: [PATCH 1/5] Restrict using Mock objects as specs. --- Lib/unittest/mock.py | 42 +++++++++++++++++++++++--- Lib/unittest/test/testmock/testmock.py | 26 ++++++++++++++-- 2 files changed, 62 insertions(+), 6 deletions(-) diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index 720f682efbb54cf..e59e606f054db9f 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -36,6 +36,10 @@ from functools import wraps, partial +class InvalidSpecError(Exception): + """Indicates that an invalid value was used as a mock spec.""" + + _builtins = {name for name in dir(builtins) if not name.startswith('_')} FILTER_DIR = True @@ -653,10 +657,17 @@ def __getattr__(self, name): self._mock_children[name] = result elif isinstance(result, _SpecState): - result = create_autospec( - result.spec, result.spec_set, result.instance, - result.parent, result.name - ) + try: + result = create_autospec( + result.spec, result.spec_set, result.instance, + result.parent, result.name + ) + except InvalidSpecError: + target_name = self.__dict__['_mock_name'] or self + raise InvalidSpecError( + f'Cannot autospec attr {name!r} from target ' + f'{target_name!r} as it has already been mocked out. ' + f'[target={self!r}, attr={result.spec!r}]') self._mock_children[name] = result return result @@ -1273,6 +1284,14 @@ def __init__( ) if not unsafe: _check_spec_arg_typos(kwargs) + if _is_instance_mock(spec): + raise InvalidSpecError( + f'Cannot spec attr {attribute!r} as the spec ' + f'has already been mocked out. [spec={spec!r}]') + if _is_instance_mock(spec_set): + raise InvalidSpecError( + f'Cannot spec attr {attribute!r} as the spec_set ' + f'target has already been mocked out. [spec_set={spec_set!r}]') self.getter = getter self.attribute = attribute @@ -1500,6 +1519,18 @@ def __enter__(self): if autospec is True: autospec = original + if _is_instance_mock(self.target): + raise InvalidSpecError( + f'Cannot autospec attr {self.attribute!r} as the patch ' + f'target has already been mocked out. ' + f'[target={self.target!r}, attr={autospec!r}]') + if _is_instance_mock(autospec): + target_name = getattr(self.target, '__name__', self.target) + raise InvalidSpecError( + f'Cannot autospec attr {self.attribute!r} from target ' + f'{target_name!r} as it has already been mocked out. ' + f'[target={self.target!r}, attr={autospec!r}]') + new = create_autospec(autospec, spec_set=spec_set, _name=self.attribute, **kwargs) elif kwargs: @@ -2613,6 +2644,9 @@ def create_autospec(spec, spec_set=False, instance=False, _parent=None, spec = type(spec) is_type = isinstance(spec, type) + if _is_instance_mock(spec): + raise InvalidSpecError(f'Cannot autospec a Mock object. ' + f'[object={spec!r}]') is_async_func = _is_async_func(spec) _kwargs = {'spec': spec} if spec_set: diff --git a/Lib/unittest/test/testmock/testmock.py b/Lib/unittest/test/testmock/testmock.py index e38f41e1d21528d..65880433cdc710f 100644 --- a/Lib/unittest/test/testmock/testmock.py +++ b/Lib/unittest/test/testmock/testmock.py @@ -11,7 +11,7 @@ call, DEFAULT, patch, sentinel, MagicMock, Mock, NonCallableMock, NonCallableMagicMock, AsyncMock, _Call, _CallList, - create_autospec + create_autospec, InvalidSpecError ) @@ -205,6 +205,28 @@ def f(): pass self.assertRaisesRegex(ValueError, 'Bazinga!', mock) + def test_autospec_mock(self): + class A(object): + class B(object): + C = None + + with mock.patch.object(A, 'B'): + with self.assertRaisesRegex(InvalidSpecError, + "Cannot autospec attr 'B' from target Date: Fri, 9 Apr 2021 22:07:55 -0400 Subject: [PATCH 2/5] Fix whitespace. --- Lib/unittest/mock.py | 2 +- Lib/unittest/test/testmock/testmock.py | 40 +++++++++++++------------- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py index e59e606f054db9f..c6067151de14fee 100644 --- a/Lib/unittest/mock.py +++ b/Lib/unittest/mock.py @@ -37,7 +37,7 @@ class InvalidSpecError(Exception): - """Indicates that an invalid value was used as a mock spec.""" + """Indicates that an invalid value was used as a mock spec.""" _builtins = {name for name in dir(builtins) if not name.startswith('_')} diff --git a/Lib/unittest/test/testmock/testmock.py b/Lib/unittest/test/testmock/testmock.py index 65880433cdc710f..fdba543b53511de 100644 --- a/Lib/unittest/test/testmock/testmock.py +++ b/Lib/unittest/test/testmock/testmock.py @@ -206,26 +206,26 @@ def f(): pass def test_autospec_mock(self): - class A(object): - class B(object): - C = None - - with mock.patch.object(A, 'B'): - with self.assertRaisesRegex(InvalidSpecError, - "Cannot autospec attr 'B' from target Date: Fri, 9 Apr 2021 23:20:34 -0400 Subject: [PATCH 3/5] Skip a broken test that exposes a bug elsewhere in mock. --- Lib/unittest/test/testmock/testasync.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/unittest/test/testmock/testasync.py b/Lib/unittest/test/testmock/testasync.py index 690ca4f55f9e3b5..81e0ce839c8b105 100644 --- a/Lib/unittest/test/testmock/testasync.py +++ b/Lib/unittest/test/testmock/testasync.py @@ -199,9 +199,9 @@ def test_create_autospec_instance(self): with self.assertRaises(RuntimeError): create_autospec(async_func, instance=True) + @unittest.skip('Broken test from bpo-issue37251') def test_create_autospec_awaitable_class(self): - awaitable_mock = create_autospec(spec=AwaitableClass()) - self.assertIsInstance(create_autospec(awaitable_mock), AsyncMock) + self.assertIsInstance(create_autospec(AwaitableClass), AsyncMock) def test_create_autospec(self): spec = create_autospec(async_func_args) From aaaeb41292b71d64f63ffe6b08e143befe8d7a22 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Fri, 9 Apr 2021 20:27:11 -0700 Subject: [PATCH 4/5] linkify bpo-issue37251 in the message. --- Lib/unittest/test/testmock/testasync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/unittest/test/testmock/testasync.py b/Lib/unittest/test/testmock/testasync.py index 81e0ce839c8b105..e1866a3492cb508 100644 --- a/Lib/unittest/test/testmock/testasync.py +++ b/Lib/unittest/test/testmock/testasync.py @@ -199,7 +199,7 @@ def test_create_autospec_instance(self): with self.assertRaises(RuntimeError): create_autospec(async_func, instance=True) - @unittest.skip('Broken test from bpo-issue37251') + @unittest.skip('Broken test from https://bugs.python.org/issue37251') def test_create_autospec_awaitable_class(self): self.assertIsInstance(create_autospec(AwaitableClass), AsyncMock) From 3b2dcdfc2dddc19cbb214e8362c7232a05962a57 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sat, 10 Apr 2021 03:30:36 +0000 Subject: [PATCH 5/5] =?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 --- .../NEWS.d/next/Library/2021-04-10-03-30-36.bpo-43478.iZcBTq.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2021-04-10-03-30-36.bpo-43478.iZcBTq.rst diff --git a/Misc/NEWS.d/next/Library/2021-04-10-03-30-36.bpo-43478.iZcBTq.rst b/Misc/NEWS.d/next/Library/2021-04-10-03-30-36.bpo-43478.iZcBTq.rst new file mode 100644 index 000000000000000..aaa1992f07fb460 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2021-04-10-03-30-36.bpo-43478.iZcBTq.rst @@ -0,0 +1 @@ +Mocks can no longer be used as the specs for other Mocks. As a result, an already-mocked object cannot have an attribute mocked using `autospec=True` or be the subject of a `create_autospec(...)` call. This can uncover bugs in tests since these Mock-derived Mocks will always pass certain tests (e.g. isinstance) and builtin assert functions (e.g. assert_called_once_with) will unconditionally pass. \ No newline at end of file