Fixes mocking inherited class methods

[nielsvaneck]

Fixes #33

Mockito loses track of the original_method. Then when the new mocked method is invoked, then invocation is handled as a class method, the first argument is removed, which is what eventually causes the TypeError that shows up in the failing tests.

This change finds the original method if a classmethod is brought in through inheritance. It also addresses a test failure against Python 2.7 that showed up after fixing the initial problem.

[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.

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 (ok, besides “indicatin”) 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.

[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.

[kaste]

Good to go, released as 1.2.1 Thanks for making this.

[kaste]

Everything okay?

[nielsvaneck]

I updated mockito in my project and it works like a charm. thank you!