kaste / mockito-python

Mockito is a spying framework
MIT License
123 stars 12 forks source link

Add support for lenient stubbed invocations #77

Open Oracking opened 6 months ago

Oracking commented 6 months ago

Current behaviour: When we call verifyStubbedInvocationsAreUsed it checks that all stubbed invocations are called at least once, ensuring we don't have unnecessary stubbing. This is often useful to have in afterEach hook. However, there are cases where we want some specific mocks to be skipped by this check. Java mockito has [lenient](https://javadoc.io/doc/org.mockito/mockito-core/latest/org/mockito/Mockito.html#lenient()) which allows a stubbed invocation to be set up without being called. This is useful in a case where we want to always mock some call in the beforeEach regardless of whether it is called in the test body (see example below). Currently, there isn't support for this.

class TestCase:
    def setUp(self):
        mockito.when("external_module").real_call_that_should_always_be_stubbed(...).thenReturn("Some response")

    def tearDown(self):
        mockito.verifyStubbedInvocationsAreUsed()

    def test_some_behaviour(self):
        # Do some stuff that don't call the stub

    def test_some_other_behaviour(self):
        # Do some stuff that call the stub

Proposed Behaviour: Thinking this can be solved in one of two ways:

  1. Have an optional lenient argument added to when, when2, expect, patch. The actual name of the argument can be something other than lenient as we currently have strict and this can be confusing. Also Java Mockito's lenient argument seems to skip argument mismatch check so having the same name (but modified behaviour) could be misleading.
  2. Relax the validity check on the atleast argument in expect so zero can be passed as a valid argument. Hence in beforeEach, user can specify mockito.expect("external_module", atleast=0).real_call_that_should_always_be_stubbed(...).thenReturn("Some response"). And this will still pass verifyStubbedInvocationsAreUsed in afterEach hook even if no calls are made to stubbed invocation in the test body.

I think Option 2 will be the least invasive but I'm keen to hear others thoughts.

I will also be happy to raise a PR with the changes required

kaste commented 6 months ago

I have something like this in the code: Look out for self.allow_zero_invocations whose value depends on

https://github.com/kaste/mockito-python/blob/766ea5c5fb8c60e8096938f597b8eed4a29b3394/mockito/invocation.py#L288-L301

iirc when times=0 or between starts at 0, i.e. between=(0,3), the stubbed method counts always as used.

kaste commented 6 months ago

I forgot to ask

This is useful in a case where we want to always mock some call in the beforeEach regardless of whether it is called in the test body (see example below).

I see the example but why do we do this? Why stubbing when you don't use it? Is that just dev ergonomics?

Oracking commented 6 months ago

Oh great! Just checked and between can be used to achieve the desired behaviour. I had tried that previously but it did not work for a different reason (it had escaped me since I'm raising this issue a few days later).

To your question - "why do we do this?" - an example scenario will be:

Say I have a TestClass that has some test cases that test function_a. Inside function_a, it conditionally invokes function_b that sends an alert. And I want to guarantee that the real alert call is never made in a test context. In this case, it feels safer to plug the mock for function_b in the setUp method. But this means some test cases that call function_a will not necessarily invoke the mocked function_b. However, it is safer because anyone who extends the test class with new test cases doesn't have to remember to mock the external service call when developing, risking unintended alerts.

Also, the reason that between had not worked for me was because expect does not work when we pass the module name as a string. I see when resolves the module it receives using get_obj. However, the same is not being done for expect. Is this intentional?

kaste commented 6 months ago

Can't see anything why expect should be different here. Mistake? Usually just change it and see if some tests are failing. (I so rarely do the strings thing in python, I think.)

Oracking commented 6 months ago

Understood. I will make the change and check if the tests run fine. If all good I will raise a PR and close issue. Appreciate the quick responses :)

kaste commented 6 months ago

When we do something with the docs, it should be added to the docs of verifyStubbedInvocationsAreUsed. That alone makes it a lot more "findable". It also makes it clear where the tests for this should be located. (Okay, tests locations are wild in this repo. LOL)

Edit: We have a test for that exact use-case:

https://github.com/kaste/mockito-python/blob/22e04513df6812c972e308f7c74877edbc3d47a0/tests/instancemethods_test.py#L303-L313