Repository: kaste/mockito-python

Issue:
Invocation of mocked @classmethod defined in super classes fails. 
I recently tried mocking a class method and saw my test failing with a cryptic message
`Fail: TypeError: missing a required argument: 'id'` and an exception somewhere in the python library code. 

I grabbed the mockito-python code, did some digging and found that the only thing that set my case apart from what mockito-python supports (and has tests for 👍) is that in my case, the class method I tried mocking, was defined on a super class of the object in `when()`. 

Here's a little example that replicates the issue: 

```python
class LoudBarker:
    @classmethod
    def loud_bark(cls, bark):
        return bark

class Dog(LoudBarker):
    @classmethod
    def bark(cls):
        return "woof!"

class LoudBarkerTest(TestBase):
    def test_loud_dog(self):
        when(Dog).loud_bark("woof!").thenReturn("WOOOOF!")
        self.assertEqual(Dog.loud_bark("woof!"), "WOOOOF!") #fails with TypeError
```

Here, `Dog.loud_bark("woof!")` will fail with `Fail: TypeError: missing a required argument: 'bark'`. 

If we write the mocking statement based on the class where the class method is defined, everything works as expected: 

```python
    def test_loud_bark__in_loud_barker(self):
        when(LoudBarker).loud_bark("woof!").thenReturn("WOOOOF!")
        self.assertEqual(Dog.loud_bark("woof!"), "WOOOOF!") #passes
```

In practice, this way of defining `when()` may be difficult because libraries do not always exactly expose in what class a class method is defined and source code may not always be available to find the answer.



Discussion:
nielsvaneck: Passes all tests on all configured Python versions: https://travis-ci.org/kaste/mockito-python/builds/651166706
kaste: Very interesting and thank you.  My first intuition was this must be a regression but I think it is not. I will leave some comments.
nielsvaneck: I reworked the tests.  https://travis-ci.org/kaste/mockito-python/builds/651216907

lmk what you think. 
kaste: I liked that simple patch, unfortunately 

```
    def testA(self):
        when(Retriever).retrieve("stick").thenReturn("ball")
        when(TrickDog).retrieve("stick").thenReturn("cat")
        unstub()
        self.assertEqual("stick", TrickDog.retrieve("stick"))
```

fails. 

![image](https://user-images.githubusercontent.com/8558/74683832-694b2f00-51ca-11ea-9b23-ce29788412db.png)

Unless I'm confused after that setup `TrickDog` is corrupted. 😞 

nielsvaneck: that is unfortunate indeed!   (from a practical point of view, it's still a good bit better than the behavior it fixes IMO)

TrickDog might be fine.  Does unstub fail to remove the modification from Retriever?  let me dig a little.

kaste: No it replaces `TrickDog.retrieve` with the patched `Retriever.retrieve` because that was the "original" method at that time.  But unstubbing must *delete* the classmethod from `TrickDog` because we basically added it. No?
nielsvaneck: Right!  So if we want to stick with the approach of stubbing at the "top" level, that leaves us with 2 choices: 
1)  when dealing with `when(TrickDog).retrieve...`  we have to actually find `retrieve` on `Retriever` and stub it there, because that's technically where the real method lives

or 

2) somehow remembering that when we're stubbing `retrieve` on `TrickDog` it technically doesn't replace anything, in which case all `unstub()` needs to do  is just remove the stub.
kaste: It's the second one otherwise a `TrickierDog` would see the mock as well.
nielsvaneck: Good point.  There seems to be a a bit of support for stubbing "nonexistent" methods, based on "original_method" being None (as you mentioned earlier)   but that's also what got us into this pickle to start with.. 

Maybe we can use original method while setting up the stub, but then not store it?  
kaste: It should be easy to return a tuple `(original_method: str, added: bool)` from `get_original_method` and then use that information in `restore_method`.  (I'm currently hitting a bug with the newest git version I just installed, so I have to restore git functionality first.) 
nielsvaneck: I'll give that a try
kaste: That's again a nice patch.  It's also very explicit via the comments that we don't know a lot here and don't model the how-to-undo-the-stubbing rather implicit. I tried to break it without success. 👍 

There is a small change in semantics for the `AttributeError` case because after this patch we always have `was_in_spec=False`. Before we set this only if we returned `None`. Hard to find a test for this, afaik as long as we don't have a `.__dict__` we also can't patch anyway. E.g. 

```
class Point:
	__slots__ = ("x", )
	def double(self): pass
```

Instances of `Point` have no `__dict__` (bc `slots`) but then `double` becomes read-only.

```
----> 1 setattr(p, "double", lambda: "foo")

AttributeError: 'Point' object attribute 'double' is read-only
```

As I said everything looks good. There are some typos in the method doc. I also tested with this

```
    def testUnStubWorksOnClassAndSuperClass(self):
        self.assertEqual("stick", Retriever.retrieve("stick"))
        self.assertEqual("stick", TrickDog.retrieve("stick"))

        when(Retriever).retrieve("stick").thenReturn("ball")
        self.assertEqual("ball", Retriever.retrieve("stick"))
        self.assertEqual("ball", TrickDog.retrieve("stick"))

        when(TrickDog).retrieve("stick").thenReturn("cat")
        self.assertEqual("ball", Retriever.retrieve("stick"))
        self.assertEqual("cat", TrickDog.retrieve("stick"))

        unstub(TrickDog)
        self.assertEqual("ball", Retriever.retrieve("stick"))
        self.assertEqual("ball", TrickDog.retrieve("stick"))

        unstub(Retriever)
        self.assertEqual("stick", Retriever.retrieve("stick"))
        self.assertEqual("stick", TrickDog.retrieve("stick"))

    def testReverseOrderWhenUnstubbing(self):
        when(Retriever).retrieve("stick").thenReturn("ball")
        when(TrickDog).retrieve("stick").thenReturn("cat")

        unstub(Retriever)
        self.assertEqual("stick", Retriever.retrieve("stick"))
        self.assertEqual("cat", TrickDog.retrieve("stick"))

        unstub(TrickDog)
        self.assertEqual("stick", Retriever.retrieve("stick"))
        self.assertEqual("stick", TrickDog.retrieve("stick"))

```

which is rather verbose but maybe better explains the "inheritance" when mocking and partially unstubbing. 


nielsvaneck: good morning! thank you for reviewing, i’ll incorporate your suggested tests. 

shall I remove the one that is commented out? (with the comment about inherited class methods not appearing in __dict__)

regarding the typos you mentioned, I read through the code again just now and nothing immediately jumps out at me as a glaring typo. perhaps it’s author goggles, or maybe it’s due to a difference in experience and verbal model of the subject matter. in any case, would you mind putting some comments in the diff on wording you’d like to see fixed? i’m happy to adopt whatever changes you suggest. 
nielsvaneck: btw, just thinking about that `AttributeError` again. in what situation would we see that?
kaste: We probably don't need the commented test.  

Do you know any other ways to get an object without a `__dict__`? Above I posted the only way I know using `slots` but this doesn't mean you can mock such a method. But it provides a clear error message.  Without the `AttributeError` catch we would crash in a way a user can't understand. It would look like a mockito bug. With it we clearly say 'x' is read-only. 


nielsvaneck: I removed the commented test. 

Also added a little comment to clarify the AttributeError. That might be helpful for the next person who comes along.. 

I cannot think of a way to make this work with slots.  unless you specify the method name you'd want to stub out in the slots collection, there's no way to overwrite them. 

Patch:
diff --git a/mockito/mocking.py b/mockito/mocking.py
index 82be9b2..0955e19 100644
--- a/mockito/mocking.py
+++ b/mockito/mocking.py
@@ -18,17 +18,14 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 # THE SOFTWARE.
 
-from collections import deque
-import inspect
 import functools
+import inspect
 import operator
+from collections import deque
 
-from . import invocation
-from . import signature
-from . import utils
+from . import invocation, signature, utils
 from .mock_registry import mock_registry
 
-
 __all__ = ['mock']
 
 __tracebackhide__ = operator.methodcaller(
@@ -75,13 +72,24 @@ class Mock(object):
     # STUBBING
 
     def get_original_method(self, method_name):
+        """
+        Looks up the original method on the `spec` object and returns it
+        together with an indication of whether the method is found
+        "directly" on the `spec` object.
+
+        This is used to decide whether the method should be stored as an
+        original_method and should therefore be replaced when unstubbing.
+        """
         if self.spec is None:
-            return None
+            return None, False
 
         try:
-            return self.spec.__dict__.get(method_name)
-        except AttributeError:
-            return getattr(self.spec, method_name, None)
+            return self.spec.__dict__[method_name], True
+        except (AttributeError, KeyError):
+            # Classes with defined `__slots__` and then no `__dict__` are not
+            # patchable but if we catch the `AttributeError` here, we get
+            # the better error message for the user.
+            return getattr(self.spec, method_name, None), False
 
     def set_method(self, method_name, new_method):
         setattr(self.mocked_obj, method_name, new_method)
@@ -130,8 +138,14 @@ class Mock(object):
         try:
             self.original_methods[method_name]
         except KeyError:
-            original_method = self.get_original_method(method_name)
-            self.original_methods[method_name] = original_method
+            original_method, was_in_spec = self.get_original_method(
+                method_name)
+            if was_in_spec:
+                # This indicates the original method was found directly on
+                # the spec object and should therefore be restored by unstub
+                self.original_methods[method_name] = original_method
+            else:
+                self.original_methods[method_name] = None
 
             self.replace_method(method_name, original_method)
 
diff --git a/mockito/utils.py b/mockito/utils.py
index ed8e7c3..52e8be4 100644
--- a/mockito/utils.py
+++ b/mockito/utils.py
@@ -14,10 +14,7 @@ def contains_strict(seq, element):
 
 
 def newmethod(fn, obj):
-    if PY3:
-        return types.MethodType(fn, obj)
-    else:
-        return types.MethodType(fn, obj, obj.__class__)
+    return types.MethodType(fn, obj)
 
 
 def get_function_host(fn):


Test patch:
diff --git a/tests/classmethods_test.py b/tests/classmethods_test.py
index 69d8cf7..0dc15bd 100644
--- a/tests/classmethods_test.py
+++ b/tests/classmethods_test.py
@@ -18,10 +18,12 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 # THE SOFTWARE.
 
-from .test_base import TestBase
-from mockito import when, unstub, verify
+from mockito import unstub, verify, when
 from mockito.verification import VerificationError
 
+from .test_base import TestBase
+
+
 class Dog:
     @classmethod
     def bark(cls):
@@ -105,3 +107,122 @@ class ClassMethodsTest(TestBase):
         self.assertEqual("Cat foo", Cat.meow("foo"))
 
 
+class Retriever:
+    @classmethod
+    def retrieve(cls, item):
+        return item
+
+
+class TrickDog(Dog, Retriever):
+    pass
+
+
+class InheritedClassMethodsTest(TestBase):
+
+    def tearDown(self):
+        unstub()
+
+    def testUnstubs(self):
+        when(TrickDog).bark().thenReturn("miau!")
+        when(TrickDog).retrieve("stick").thenReturn("ball")
+        unstub()
+        self.assertEqual("woof!", TrickDog.bark())
+        self.assertEqual("stick", TrickDog.retrieve("stick"))
+
+    def testStubs(self):
+        self.assertEqual("woof!", TrickDog.bark())
+        self.assertEqual("stick", TrickDog.retrieve("stick"))
+
+        when(TrickDog).bark().thenReturn("miau!")
+        when(TrickDog).retrieve("stick").thenReturn("ball")
+
+        self.assertEqual("miau!", TrickDog.bark())
+        self.assertEqual("ball", TrickDog.retrieve("stick"))
+
+    def testVerifiesMultipleCallsOnClassmethod(self):
+        when(TrickDog).bark().thenReturn("miau!")
+        when(TrickDog).retrieve("stick").thenReturn("ball")
+
+        TrickDog.bark()
+        TrickDog.bark()
+
+        TrickDog.retrieve("stick")
+        TrickDog.retrieve("stick")
+
+        verify(TrickDog, times=2).bark()
+        verify(TrickDog, times=2).retrieve("stick")
+
+    def testFailsVerificationOfMultipleCallsOnClassmethod(self):
+        when(TrickDog).bark().thenReturn("miau!")
+        when(TrickDog).retrieve("stick").thenReturn("bark")
+
+        TrickDog.bark()
+        TrickDog.retrieve("stick")
+
+        self.assertRaises(VerificationError, verify(TrickDog, times=2).bark)
+        self.assertRaises(VerificationError, verify(TrickDog,
+                                                    times=2).retrieve)
+
+    def testStubsAndVerifiesClassmethod(self):
+        when(TrickDog).bark().thenReturn("miau!")
+        when(TrickDog).retrieve("stick").thenReturn("ball")
+
+        self.assertEqual("miau!", TrickDog.bark())
+        self.assertEqual("ball", TrickDog.retrieve("stick"))
+
+        verify(TrickDog).bark()
+        verify(TrickDog).retrieve("stick")
+
+    def testPreservesSuperClassClassMethodWhenStubbed(self):
+        self.assertEqual("woof!", Dog.bark())
+        self.assertEqual("stick", Retriever.retrieve("stick"))
+
+        self.assertEqual("woof!", TrickDog.bark())
+        self.assertEqual("stick", TrickDog.retrieve("stick"))
+
+        when(TrickDog).bark().thenReturn("miau!")
+        when(TrickDog).retrieve("stick").thenReturn("ball")
+
+        self.assertEqual("miau!", TrickDog.bark())
+        self.assertEqual("ball", TrickDog.retrieve("stick"))
+
+        self.assertEqual("woof!", Dog.bark())
+        self.assertEqual("stick", Retriever.retrieve("stick"))
+
+    def testDoubleStubStubWorksAfterUnstub(self):
+        when(TrickDog).retrieve("stick").thenReturn("ball")
+        when(TrickDog).retrieve("stick").thenReturn("cat")
+        unstub()
+        self.assertEqual("stick", TrickDog.retrieve("stick"))
+
+    def testUnStubWorksOnClassAndSuperClass(self):
+        self.assertEqual("stick", Retriever.retrieve("stick"))
+        self.assertEqual("stick", TrickDog.retrieve("stick"))
+
+        when(Retriever).retrieve("stick").thenReturn("ball")
+        self.assertEqual("ball", Retriever.retrieve("stick"))
+        self.assertEqual("ball", TrickDog.retrieve("stick"))
+
+        when(TrickDog).retrieve("stick").thenReturn("cat")
+        self.assertEqual("ball", Retriever.retrieve("stick"))
+        self.assertEqual("cat", TrickDog.retrieve("stick"))
+
+        unstub(TrickDog)
+        self.assertEqual("ball", Retriever.retrieve("stick"))
+        self.assertEqual("ball", TrickDog.retrieve("stick"))
+
+        unstub(Retriever)
+        self.assertEqual("stick", Retriever.retrieve("stick"))
+        self.assertEqual("stick", TrickDog.retrieve("stick"))
+
+    def testReverseOrderWhenUnstubbing(self):
+        when(Retriever).retrieve("stick").thenReturn("ball")
+        when(TrickDog).retrieve("stick").thenReturn("cat")
+
+        unstub(Retriever)
+        self.assertEqual("stick", Retriever.retrieve("stick"))
+        self.assertEqual("cat", TrickDog.retrieve("stick"))
+
+        unstub(TrickDog)
+        self.assertEqual("stick", Retriever.retrieve("stick"))
+        self.assertEqual("stick", TrickDog.retrieve("stick"))