new bug introduced/functionality removed in upgrade from 1.3 to 1.4

[jan-spurny]

I just upgraded from mockito-1.3.0 to mockito-1.4.0 and many of my tests began to fail. Here is "somewhat minimal" example of what kind of code used to work, but it doesn't anymore (python 3.7.3):

from typing import Callable
from nose.tools import assert_equal
from mockito import mock, when


class _c(object):
    def __init__(self, fn: Callable[[int], str]) -> None:
        self._fn = fn

    def get_val(self, a: int) -> str:
        return self._fn(a)


def test_foo():
    x = mock()
    c = _c(x.get_value)
    when(x).get_value(10).thenReturn('foo')
    assert_equal(c.get_val(10), 'foo')

result:

$ nosetests a.py 
F
======================================================================
FAIL: a.test_foo
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/spurny/py-venv/wf3/lib/python3.7/site-packages/nose/case.py", line 198, in runTest
    self.test(*self.arg)
  File "/home/spurny/src-experiments/mockito-1.4-bug/a.py", line 18, in test_foo
    assert_equal(c.get_val(10), 'foo')
AssertionError: None != 'foo'
----------------------------------------------------------------------
Ran 1 test in 0.000s
FAILED (failures=1)

but if I mock the x.get_value before creating c:

...
def test_foo():
    x = mock()
    when(x).get_value(10).thenReturn('foo')  # moved here from below `c = _c(...` line
    c = _c(x.get_value)
    assert_equal(c.get_val(10), 'foo')

then it works fine:

$ nosetests a.py 
.
----------------------------------------------------------------------
Ran 1 test in 0.000s
OK

[kaste]

Thanks for reporting. I think a equivalent reduced test case is just

from mockito import mock, when


def test_foo():
    x = mock()
    f1 = x.get_value
    when(x).get_value(10).thenReturn('foo')
    assert f1(10) == 'foo'

So the point is we detach a method which is not configured yet. Then we configure the mock but invoke the detached function (f1 here).

We actually have tests around detaching methods and obviously and sadly they did not catch fire here.

First bad commit is 77dda37 (Implement thenCallOriginalImplementation) which implements the main feature of 1.4.0 It looks like eat_self returns the wrong answer here (True) so that the call f1(10) looses its first argument and becomes just f1(). This doesn't match so the answer is None because the mock x is not strict and always returns None for unconfigured calls.

I hope the summary is correct. I don't see anything very obviously wrong in the mockito code.

[kaste]

It looks though like a special case here as

class F:
    def get_value(self, val):
        return val

def test_detached_early():
    f = F()
    f1 = f.get_value
    when(f).get_value(10).thenReturn('foo')
    assert f1(10) == 'foo'

doesn't work in 1.3.0. Only mocks behave in such a way.

It looks odd as roughly the code does

x = mock()
fn = x.foo
x.foo = lambda *a, **kw: 'foo'
fn(10)

Basically the when translates to monkey-patching the given object. But since mocks aren't full objects when they're non-strict and unconfigured they just store the method name on detaching and late resolve to an actual answer (function), like always return 'foo'.

You could easily argue that f1 should return None here always when it is detached before configuration. Because that was the state of the mock at that time. Not that we deeply follow this idea either; for example you cannot detach a method multiple times and make them behave differently. At least, I don't think I can make this work for the case of patching classes/instances. (The F() case right above.) I do have an probably ugly patch to "fix" the regression but I cannot make it coherent. Basically it is coherent right now. The early detach would still not work for strict mocks, classes, instances, plain functions.

[jan-spurny]

I've already rewritten my failing tests (I had no choice), so if it turns out that this is technically a regression, but in reality, this is how most people expect it to behave, it's probably better to keep the current (1.4) behaviour, rather than make "ugly patches"..

I reported this just because I used this "pattern" (passing mock's not-yet-mocked method as a function to some tested code) a lot from several years ago and it worked fine accross multiple platforms and python versions, so when it started failing, I didn't thought about what if I was doing something "fishy" and just reported it.

Btw, what would be a better way to "mock" a function to be passed to some code? I know I can use some normal functions/lambdas and do parameters checking in them, but that's why I'm using mockito, so I don't have to do this manually..

[kaste]

You mean a different way from your original example? Typically just pass in a mock(). Then early or late configuration works:

class _c(object):
    def __init__(self, fn):
        self._fn = fn

    def get_val(self, a):
        return self._fn(a)


def test_foo():
    x = mock()
    c = _c(x)
    when(x).__call__(10).thenReturn('foo')
    assert c.get_val(10) == 'foo'

In that case you can also turn on "strictness" so it has a better error message.

(Otherwise you always have the None is not 'foo' assertion error.)

[jan-spurny]

Perfect, thanks! I didn't thought of mocking __call__..