bpo-43478: Restrict use of Mock objects as specs (GH-25326)

msuozzo · web-flow · commit dccdc500f9b5 · 2021-04-09T20:45:50.000-07:00
* Restrict using Mock objects as specs as this is always a test bug where the resulting mock is misleadingly useless.
* Skip a broken test that exposes a bug elsewhere in mock (noted in the original issue).
diff --git 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/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 https://bugs.python.org/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)
diff --git 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 <MagicMock spec='A'"):
+                create_autospec(A).B
+            with self.assertRaisesRegex(InvalidSpecError,
+                                        "Cannot autospec attr 'B' from target 'A'"):
+                mock.patch.object(A, 'B', autospec=True).start()
+            with self.assertRaisesRegex(InvalidSpecError,
+                                        "Cannot autospec attr 'C' as the patch target "):
+                mock.patch.object(A.B, 'C', autospec=True).start()
+            with self.assertRaisesRegex(InvalidSpecError,
+                                        "Cannot spec attr 'B' as the spec "):
+                mock.patch.object(A, 'B', spec=A.B).start()
+            with self.assertRaisesRegex(InvalidSpecError,
+                                        "Cannot spec attr 'B' as the spec_set "):
+                mock.patch.object(A, 'B', spec_set=A.B).start()
+
     def test_reset_mock(self):
         parent = Mock()
         spec = ["something"]
@@ -2177,7 +2199,7 @@ def __init__(self):
                 self.obj_with_bool_func = unittest.mock.MagicMock()
 
         obj = Something()
-        with unittest.mock.patch.object(obj, 'obj_with_bool_func', autospec=True): pass
+        with unittest.mock.patch.object(obj, 'obj_with_bool_func', spec=object): pass
 
         self.assertEqual(obj.obj_with_bool_func.__bool__.call_count, 0)
 
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
@@ -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.

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`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.