2.0.0 checking behaviour is confusing

[Kyle-sandeman-mrdfood]

Good day

In version 2 it seems all context managers now verify whether when() clauses were used:

@pytest.fixture(autouse=True)
def mock_external():
  with when(some_lib).request(...).thenReturn('mock value'):
    yield

Now results in mockito.verification.VerificationError (unused stub)
While the docs do briefly cover this with the between=(0,) part, it is highly confusing - I didn't use any verification function here specifically because tests may or may not encounter this stub.

Please assist with clarity here? Are the verification functions only needed if you don't use context managers?

Maybe opinionated, but the current docs for e.g. expect seem to encourage a fragile paradigm. Why not promote context managers?
Existing docs:

# Given `dog` is an instance of a `Dog`
expect(dog, times=1).bark('Wuff').thenReturn('Miau')
dog.bark('Wuff')
dog.bark('Wuff')  # will throw at call time: too many invocations

# maybe if you need to ensure that `dog.bark()` was called at all
verifyExpectedInteractions()

becomes

with expect(dog, times=1).bark('Wuff').thenReturn('Miau'):
    dog.bark('Wuff')
    dog.bark('Wuff')  # will throw at call time: too many invocations

# OR

expect(dog, times=1).bark('Wuff').thenReturn('Miau')
dog.bark('Wuff')
dog.bark('Wuff')  # will throw at call time: too many invocations
# When not using the context manager, you will likely want to verify it was called at all:
verifyExpectedInteractions()

Apologies if I missed the crucial part explaining this difference, I skim read the docs again after the update but didn't see it.
I would definitely think the "context manager vs not" paradigms should be explained (esp. regarding which verifications are automatic)

Thank you for the wonderful library!

[Kyle-sandeman-mrdfood]

I see that (at least with a context manager) that expect with no verification args is effectively equal to passing atleast=1. Which is effectively equal to when. Is this correct?

[kaste]

I think I forgot the docs (a bit at least) as the async/await, property/descriptor, and chaining/fluent API's support was so difficult and challenging. 😁

Fact is that the context managers were the introduction in the v1 line; the hope was/is to have them better and stable now. The checking behavior of the context managers can be controlled using the environment var MOCKITO_CONTEXT_MANAGERS_CHECK_USAGE.

IMO with usage justifies the check because these syntax constructs are local. (Compared to a configuration you do in a setUp/fixture phase). At a minimum with expect().foo() reads like (again IMO) I expect that the block uses foo(). with when less so.

[kaste]

I do see that you have

@pytest.fixture(autouse=True)
def mock_external():
  with when(some_lib).request(...).thenReturn('mock value'):
    yield

specifically autouse. That was part of an issue and lead to
https://mockito-python.readthedocs.io/en/latest/recipes.html#shared-setup-stubs-with-teardown-safety-checks actually (with the lower/zero bound between usage being implemented for it).

[Kyle-sandeman-mrdfood]

100%, especially on the IMO ... part.
To me it isn't clear that this is how it works, which led to the confusion. I only use mockito with context managers so the docs need to be taken with a pinch of salt. For example I was still doing verifyNoUnwantedInteractions() before exiting every context manager...
It would be great to have a page dedicated to explaining how context manager usage differs (and perhaps is even recommended, up to you). In that section of setup/teardown a pytest fixture example with a context manager would also go a long way for future readers.
I could give a stab at this in a PR, if you can give me some guidelines.

In any case, I did migrate my one test suite to v2 without too much trouble. It is now cleaner and more explicit 🎉

[jan-spurny]

It would really help to have more detailed: "how to fix tests broken by v2" guide - In our team, we will probably spent several days fixing everything that was broken... and as of now, I'm still finding new problems which I don't know how to fix.. or what the problem was...

I don't want to sound ungrateful - I am grateful for python mockito and I'm using it from the start.. It's just that this broke everything on which I learn to depend..

[kaste]

Hey, just write down what's broken, maybe it actually is broken and not an intentional change.

I can't remember I even removed unit tests, at least en masse, while developing v2. Just some deprecations.

[jan-spurny]

Ok, I'll try, but it's a bit complicated, since I haven't figured out what is the problem (and creating minimal working example from some of our tests would really take too much time.. there are literally hundreds of tests which are not working with v2 and we are on deadlines...):

1. we have specialized matcher for numpy arrays extending ArgThat - the interface of which has changed - this was no problem to fix, but it would be nice to mention that matchers are different
2. most frustrating was a code where we have (in multiple files) things like:

from foo.bar.baz import myfunc
import foo.bar.baz as m

@fixture
def result1(a, b):
    with when(m.np).vstack([a, b]).thenReturn(result1 := mock()):
        yield result1

@fixture
def result2(c, d):
    with when(m.np).vstack([c, d]).thenReturn(result2 := mock()):
        yield result2

def test_passing_values(a, b, c, d, result1, result2):
    assert myfunc(a, b, c, d) == [result1, result2]

and there was error "mockito.invocation.InvocationError: 'vstack' is not callable"

sorry for not including minimal example - the problem with minimal example is that it works.. and real code doesnt - and I've spent last 4 hours figuring out why - there are 3 fixtures with when(m.np).vstack(... but only one fails... no matter where I moved it, no matter how I rearranged fixtures... no matter how I tried to wrap m.np as spy or import m separately in each fixture...

To be fair, it could be fixed by when2 but one of the reasons for using mockito (at least for us) was the clarity of when(object).function(a, b, c).thenReturn(x) notation - there is clear distinction of what a function is and what arguments are - in when2 it's all mashed together and tests look like ugly hacks...

3. we heavily used pattern:

with when(foo).bar(...):
    myfunc()
    verify(foo, times=0)

which is now impossible - it's not a problem to change it to expect(foo, times=0).bar(...) but it takes time...

There may be other problems, and I believe they are fixable, but it's just not worth it for us at the time..

[kaste]

1. I don't remember anything changing around arg_that. I don't think anything has changed for ArgThat as well, but the latter is actually private. captor has changed.
2. 'vstack' is not callable looks like a bug/regression.
3. Not sure what you mean here. verify(foo, times=0) without a method was a feature?

[kaste]

vstack has been fixed in v2.0.1

I think verify(foo, times=0) is a no-op and was so before. You don't have to switch to expect, verify(foo, times=0).bar(...) works as well. You can also configure mockito to skip that test using MOCKITO_CONTEXT_MANAGERS_CHECK_USAGE.

[kaste]

Possible variants are to allow

with when(some_lib, strict=False).request(...).thenReturn('mock value'): pass  
with when(some_lib).request(...).thenReturn('mock value').mayBeUnused(): pass  

We already have the kwarg strict, just with the narrower meaning: do not check against a spec e.g. for signature mismatches.

But we also have in general, the distinction when vs expect, and here with expect()... just reads stricter than with when; so this could justify a semantic nuance between them.

[jan-spurny]

Thanks for quick replies! And it seems that after 2.0.1 fixes a lot of problems for us, so thanks!

The rest is just a clarification of what I wrote earlier:

sorry ad verify(foo, times=0) - what I meant was: verify(foo, times=0).bar(...) and from that, we got many errors with unused stub - but I wouldn't worry about that - if I found some specific error which can't be easily worked around, I'll make a separate issue with minimal working example, would that be ok?

ad arg_that - the difference is that for the old one, we had:

def _np_eq(val: np.ndarray | float) -> ArgThat:
    class VerboseArgThat(ArgThat):
        def __repr__(self) -> str:
            return f'ArgThat(={val})'
        def __str__(self) -> str:
            return repr(self)

    return VerboseArgThat(lambda x: np.allclose(x, val))

and that doesn't work in 2.0.0, so we changed it to:

def _np_eq(val: np.ndarray | float) -> ArgThat:
    class VerboseArgThat(ArgThat):
        def __init__(self) -> None:
            pass
        def __repr__(self) -> str:
            return f'ArgThat(={val})'
        def __str__(self) -> str:
            return repr(self)
        def matches(self, x: Any) -> bool: # noqa ANN001 - need any value
            try:
                return np.allclose(x, val)
            except (TypeError, ValueError):
                return False

    return VerboseArgThat()

which works fine - so I don't see it as a problem as it's just one fix for each of our custom matchers and it works, I'm not complaining, just explaining.
But I do think it should be mentioned in "breaking changes" - something like - "if you made your own custom matchers, they may not work"

And I can confirm that there is less broken code with 2.0.1 - thanks!