Fix various bugs around spec arguments to patchers

4 files changed, 102 insertions, 26 deletions

diff --git a/docs/changelog.txt b/docs/changelog.txt
index 8e12639..4eb3fa0 100644
--- a/docs/changelog.txt
+++ b/docs/changelog.txt
@@ -20,8 +20,12 @@ The standard library version!
   methods (other than equality and inequality) now return `NotImplemented`
 * Propagate traceback info to support subclassing of `_patch` by other
   libraries
+* BUGFIX: passing multiple spec arguments to patchers (`spec` , `spec_set` and
+  `autospec`) had unpredictable results, now it is an error
 * BUGFIX: using `spec=True` *and* `create=True` as arguments to patchers could
   result in using `DEFAULT` as the spec. Now it is an error instead
+* BUGFIX: using `spec` or `autospec` arguments to patchers, along with
+  `spec_set=True` did not work correctly
 
 
 2012/02/13 Version 0.8.0
diff --git a/docs/patch.txt b/docs/patch.txt
index 190fa13..d27dbc5 100644
--- a/docs/patch.txt
+++ b/docs/patch.txt
@@ -40,7 +40,7 @@ patch
     `patch` is straightforward to use. The key is to do the patching in the
     right namespace. See the section `where to patch`_.
 
-.. function:: patch(target, new=DEFAULT, spec=None, create=False, spec_set=None, autospec=False, new_callable=None, **kwargs)
+.. function:: patch(target, new=DEFAULT, spec=None, create=False, spec_set=None, autospec=None, new_callable=None, **kwargs)
 
     `patch` acts as a function decorator, class decorator or a context
     manager. Inside the body of the function or with statement, the `target`
@@ -208,7 +208,7 @@ into a `patch` call using `**`:
 patch.object
 ============
 
-.. function:: patch.object(target, attribute, new=DEFAULT, spec=None, create=False, spec_set=None, autospec=False, new_callable=None, **kwargs)
+.. function:: patch.object(target, attribute, new=DEFAULT, spec=None, create=False, spec_set=None, autospec=None, new_callable=None, **kwargs)
 
     patch the named member (`attribute`) on an object (`target`) with a mock
     object.
@@ -336,7 +336,7 @@ magic methods `__getitem__`, `__setitem__`, `__delitem__` and either
 patch.multiple
 ==============
 
-.. function:: patch.multiple(target, spec=None, create=False, spec_set=None, autospec=False, new_callable=None, **kwargs)
+.. function:: patch.multiple(target, spec=None, create=False, spec_set=None, autospec=None, new_callable=None, **kwargs)
 
     Perform multiple patches in a single call. It takes the object to be
     patched (either as an object or a string to fetch the object by importing)
diff --git a/mock.py b/mock.py
index 75aaf73..e852bcf 100644
--- a/mock.py
+++ b/mock.py
@@ -1089,7 +1089,7 @@ class _patch(object):
                 raise ValueError(
                     "Cannot use 'new' and 'new_callable' together"
                 )
-            if autospec is not False:
+            if autospec is not None:
                 raise ValueError(
                     "Cannot use 'autospec' and 'new_callable' together"
                 )
@@ -1218,17 +1218,38 @@ class _patch(object):
         new_callable = self.new_callable
         self.target = self.getter()
 
+        # normalise False to None
+        if spec is False:
+            spec = None
+        if spec_set is False:
+            spec_set = None
+        if autospec is False:
+            autospec = None
+
+        if spec is not None and autospec is not None:
+            raise TypeError("Can't specify spec and autospec")
+        if ((spec is not None or autospec is not None) and
+            spec_set not in (True, None)):
+            raise TypeError("Can't provide explicit spec_set *and* spec or autospec")
+
         original, local = self.get_original()
 
-        if new is DEFAULT and autospec is False:
+        if new is DEFAULT and autospec is None:
             inherit = False
-            if spec_set == True:
-                spec_set = original
-            elif spec == True:
+            if spec is True:
                 # set spec to the object we are replacing
                 spec = original
+                if spec_set is True:
+                    spec_set = original
+                    spec = None
+            elif spec is not None:
+                if spec_set is True:
+                    spec_set = spec
+                    spec = None
+            elif spec_set is True:
+                spec_set = original
 
-            if (spec or spec_set) is not None:
+            if spec is not None or spec_set is not None:
                 if original is DEFAULT:
                     raise TypeError("Can't use 'spec' with create=True")
                 if isinstance(original, ClassTypes):
@@ -1259,14 +1280,17 @@ class _patch(object):
             if inherit and _is_instance_mock(new):
                 # we can only tell if the instance should be callable if the
                 # spec is not a list
-                if (not _is_list(spec or spec_set) and not
-                    _instance_callable(spec or spec_set)):
+                this_spec = spec
+                if spec_set is not None:
+                    this_spec = spec_set
+                if (not _is_list(this_spec) and not
+                    _instance_callable(this_spec)):
                     Klass = NonCallableMagicMock
 
                 _kwargs.pop('name')
                 new.return_value = Klass(_new_parent=new, _new_name='()',
                                          **_kwargs)
-        elif autospec is not False:
+        elif autospec is not None:
             # spec is ignored, new *must* be default, spec_set is treated
             # as a boolean. Should we check spec is not None and that spec_set
             # is a bool?
@@ -1343,13 +1367,12 @@ def _get_target(target):
 
 def _patch_object(
         target, attribute, new=DEFAULT, spec=None,
-        create=False, spec_set=None, autospec=False,
+        create=False, spec_set=None, autospec=None,
         new_callable=None, **kwargs
     ):
     """
     patch.object(target, attribute, new=DEFAULT, spec=None, create=False,
-                 spec_set=None, autospec=False,
-                 new_callable=None, **kwargs)
+                 spec_set=None, autospec=None, new_callable=None, **kwargs)
 
     patch the named member (`attribute`) on an object (`target`) with a mock
     object.
@@ -1370,9 +1393,8 @@ def _patch_object(
     )
 
 
-def _patch_multiple(target, spec=None, create=False,
-        spec_set=None, autospec=False,
-        new_callable=None, **kwargs
+def _patch_multiple(target, spec=None, create=False, spec_set=None,
+                    autospec=None, new_callable=None, **kwargs
     ):
     """Perform multiple patches in a single call. It takes the object to be
     patched (either as an object or a string to fetch the object by importing)
@@ -1423,8 +1445,7 @@ def _patch_multiple(target, spec=None, create=False,
 
 def patch(
         target, new=DEFAULT, spec=None, create=False,
-        spec_set=None, autospec=False,
-        new_callable=None, **kwargs
+        spec_set=None, autospec=None, new_callable=None, **kwargs
     ):
     """
     `patch` acts as a function decorator, class decorator or a context
diff --git a/tests/testpatch.py b/tests/testpatch.py
index d6ebec3..5f6a1b1 100644
--- a/tests/testpatch.py
+++ b/tests/testpatch.py
@@ -20,6 +20,7 @@ if inPy3k:
     unicode = str
 
 PTModule = sys.modules[__name__]
+MODNAME = '%s.PTModule' % __name__
 
 
 def _get_proxy(obj, get_only=True):
@@ -687,13 +688,13 @@ class PatchTest(unittest2.TestCase):
 
 
     def test_patch_spec_set(self):
-        @patch('%s.SomeClass' % __name__, spec=SomeClass, spec_set=True)
+        @patch('%s.SomeClass' % __name__, spec_set=SomeClass)
         def test(MockClass):
             MockClass.z = 'foo'
 
         self.assertRaises(AttributeError, test)
 
-        @patch.object(support, 'SomeClass', spec=SomeClass, spec_set=True)
+        @patch.object(support, 'SomeClass', spec_set=SomeClass)
         def test(MockClass):
             MockClass.z = 'foo'
 
@@ -1653,7 +1654,6 @@ class PatchTest(unittest2.TestCase):
 
 
     def test_patch_propogrates_exc_on_exit(self):
-
         class holder:
             exc_info = None, None, None
 
@@ -1666,8 +1666,8 @@ class PatchTest(unittest2.TestCase):
         def with_custom_patch(target):
             getter, attribute = _get_target(target)
             return custom_patch(
-                getter, attribute, DEFAULT, None, False, False,
-                False, None, {}
+                getter, attribute, DEFAULT, None, False, None,
+                None, None, {}
             )
 
         @with_custom_patch('squizz.squozz')
@@ -1690,7 +1690,7 @@ class PatchTest(unittest2.TestCase):
             self.assertRaises(NameError, lambda: doesnotexist)
 
             # check that spec with create is innocuous if the original exists
-            p = patch('%s.PTModule' % __name__, create=True,
+            p = patch(MODNAME, create=True,
                       **{kwarg: True})
             try:
                 p.start()
@@ -1698,5 +1698,56 @@ class PatchTest(unittest2.TestCase):
                 p.stop()
 
 
+    def test_multiple_specs(self):
+        original = PTModule
+        for kwarg in ('spec', 'spec_set'):
+            p = patch(MODNAME, autospec=0, **{kwarg: 0})
+            self.assertRaises(TypeError, p.start)
+            self.assertIs(PTModule, original)
+
+        for kwarg in ('spec', 'autospec'):
+            p = patch(MODNAME, spec_set=0, **{kwarg: 0})
+            self.assertRaises(TypeError, p.start)
+            self.assertIs(PTModule, original)
+
+        for kwarg in ('spec_set', 'autospec'):
+            p = patch(MODNAME, spec=0, **{kwarg: 0})
+            self.assertRaises(TypeError, p.start)
+            self.assertIs(PTModule, original)
+
+
+    def test_specs_false_instead_of_none(self):
+        p = patch(MODNAME, spec=False, spec_set=False, autospec=False)
+        mock = p.start()
+        try:
+            # no spec should have been set, so attribute access should not fail
+            mock.does_not_exist
+            mock.does_not_exist = 3
+        finally:
+            p.stop()
+
+
+    def test_falsey_spec(self):
+        for kwarg in ('spec', 'autospec', 'spec_set'):
+            p = patch(MODNAME, **{kwarg: 0})
+            m = p.start()
+            try:
+                self.assertRaises(AttributeError, getattr, m, 'doesnotexit')
+            finally:
+                p.stop()
+
+
+    def test_spec_set_true(self):
+        for kwarg in ('spec', 'autospec'):
+            p = patch(MODNAME, spec_set=True, **{kwarg: True})
+            m = p.start()
+            try:
+                self.assertRaises(AttributeError, setattr, m,
+                                  'doesnotexist', 'something')
+                self.assertRaises(AttributeError, getattr, m, 'doesnotexist')
+            finally:
+                p.stop()
+
+
 if __name__ == '__main__':
     unittest2.main()