Implementation of thenCallOriginalImplementation

[avandierast]

The implementation of thenCallOriginalImplementation required a refactor of the way self is discarded. Now, self is kept in the params and discarded only when we don't need it.

Linked to #46 #48

I've also corrected some tests where the objects was used instead of the mock.

[kaste]

Very promising. I wouldn't mind if you split the commit. The fix of the ellipsis test can be one commit; the small additional test in staticmethods_test can be one separate commit as well.

You need to add tests for using thenCallOriginalImplementation stubbing classmethods and staticmethods. Typically a chaining test case like when(os.path).exists(__file__).thenReturn(False).thenCallOriginalImplementation() too.

Also define what happens if you use it on a method/function not present on the original spec/object.

The patch is easy too read. I do not like that we wildly walk the objects so deep, e.g. invocation.mock.mocked_obj or even invocation.mock.mocked_obj.__dict__ . I wonder why you use mocked_obj here, in Mock.get_original_method and RememberedProxyInvocation.__call__ we access mock.spec for the original method/implementation. Ideally the original implementation could be found on mock.original_methods[method_name] but we often store Nones here so that would need a different refactoring before.

I think it would be better if we decide that we must cut the first argument earlier. We actually cut the argument late as you do here but decide if we have to early. For example it must be clear when we construct AnswerSelector that we must cut self. (E.g. AnswerSelector(invocation, cut_self=True).) -- because the answer only depends on the used method name (invocation.method_name) and the mock (or mock.spec). We can compute a cut_first_argument when we construct the Invocation probably.

That being said; usually I'm happy with lots of tests so we can try different factorings, different ways to write the same thing, safely and later.

[avandierast]

Hello :)

Thanks for the detailed response. The idea with AnswerSelector(..., cut_first_argument=...) seems great to simplify the code. The chained sub-objects wild walk and the access to dict may become useless with an implementation like this. I'll try to implement that.

[avandierast]

Ok, deciding to cut self a bit earlier doesn't change the way we can retrieve the original function.

I didn't see .original_methods[] and it may be the cleanest way to get the original but it is mostly used for the unstubbing and thus is not really reliable as you said. We can't use get_original_method because it doesn't return the original implementation anymore during the stubbing...

Trying to get the original method on mock.spec doesn't reduce the complexity of the code since it's literally the same object as mocked_obj most of the time and when it is not, we have to read mocked_obj to get what we need.

Finally, after lots of attempt, I've decided to do a slight refacto of Mock to separate the unstub mecanism from the original methods retrieval.

The new implementation seems really more clean ! Maybe there is stil way for improvement or usefull tests to check. Let me know :)

If you prefer the refacto in another PR that's also possible.

It still remains the definition of what happens when the mocked attribute is not present.

Adrian

[kaste]

This is a lot harder to read than the first version. This will take a sec.

General note as we need to stay green per commit: you can always add tests before implementation but mark them as xfail. We should put all tests for this feature in one file otherwise it gets confusing.

[avandierast]

Hello :)

Ok, your comment made things even clearer to me ! You're right, we only need to potentialy discard self in thenAnswer. I've modified the code, it works well.

About should_discard_self, it needs new_mocked_method because of the mock() in the tests... In those tests, the functions are added afterwards and there is no spec... And since users of mockito-python may have also done mocks that are not strict, I have tried to make it work without changing the tests. But, if I move should_discard_self inside Mock, rename get_mocked_method in _get_new_mocked_method and use the original code for the test, it works also well ! Don't know why I needed inspect.isclass(mocked_method), maybe what has remained from previous tries on the code.

I let you look at it and tell me what you think :)

[kaste]

Ok, doing should_discard_self based on the original_method is in tricky territory, but this one works for me:

    def have_prepended_arg(self, method_name):
        """ Returns if the method will have a prepedend self/class argument on call."""
        try:
            original_method = self._original_methods[method_name]
        except KeyError:
            return False
        else:
            return (
                inspect.ismethod(original_method)
                or (
                    inspect.isclass(self.mocked_obj)
                    and not isinstance(original_method, staticmethod)
                    and not inspect.isclass(original_method)
                )
            )

have_prepended_arg is not the better name. It should be "has" not "have" anyway? Usually skip_first_arg which is the same as discard_first_arg you already use. Or eat_self. Btw, I think the reference to a self is okay here even for classmethods because Python uses __self__ in both cases. And that we users name the first argument cls is really conventional only as we can give it any name we want.

[avandierast]

Btw, I think the reference to a self is okay here even for classmethods because Python uses self in both cases.

Ok, I changed because of classmethod :)

[avandierast]

Ok, your implementation with the orignal_method works well. I've added a test on non-strict module function stubbing.

But it assumes that if original_method is None, then the new method can only be in two categories: module function and classic method. That's true because the when interface don't let us do anything else. I've added a comment about that because it seems usefull to not let that implicit.

Adrian

[kaste]

I think this is okay. Honestly I tried to implement this before but failed.

Unfortunately there is a huge "But...". The commits are not readable or tell a story. You also merged master back in which makes git rebase a bit harder. I cannot just squash because there are unrelated things in this PR. I cannot simply cherry-pick as the single commits mix different matters as well. (For example b191137 is a fixup for various other commits meshed into one.)

I think what you should have done is push fixup! or squash! commits so there would be no need to push --force multiple times and we could apply them now to get better commits.

1. It looks like you mangle the import section when you touch a file. I do like the import style of your tool/script, or you do that manually? But it is not a concern here. Do this in one commit in a different PR or as the first commit I could cherry-pick.
2. Commit 5e8534a must be dropped. We use m = mock(); m.strict = False here because we need a fake object as the first argument to StubbedInvocation. There is no need to call it with a "real" Mock() object, we're in the tests and want to fake the input here a bit.

If I see it correctly there are 6 commits you want to add to the project:

1. Restyle import section
2. Add a test to tests/staticmethods_test.py
3. Fix a typo in a method name in tests/stubbing_test.py
4. Add an (important!) test to modulefunctions_test.py
5. Rename self.original_methods-> self._methods_to_unstub
6. Implement thenCallOriginalImplementation

I probably should add comments inline.

Tell me how your git skills are before I go down the partial cherry pick patch hole.

[avandierast]

Hello :)

Let's clean up that PR commits :D I "think" my git skills are enough so I will do it.

For the import reordering, I use isort. (I'm also a huge fan of black)

[kaste]

That's the default configuration/style from isort?

I was an early adaptor of black, but they just never really evolved. It just produces too many really bad, ill formatted lines. (It should really produce none ill formatted lines to be clear because otherwise it becomes an annoyance fast.) I also liked the obviously javascript informed style of black first, but doing a lot more python than before I actually find the python standard library style would be most of the time👍.

[avandierast]

For isort, I use the last version with the black profile :D But the profile doesn't seem to change anything here. So yes, I think that is the default configuration.

I have not applied isort to the whole project, maybe you were expecting that for the first commit.

[kaste]

❤️ it. Great work.

[avandierast]

Thanks ! Great review ;)

[kaste]

Here we go.