New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: memorizing checked calls and allow to verify no more interactions to make it similar to Mockito verifyNoMoreInteractions() #207

Closed
lukaszkalnik opened this Issue Dec 26, 2018 · 28 comments

Comments

Projects
None yet
2 participants
@lukaszkalnik
Copy link

lukaszkalnik commented Dec 26, 2018

I have read other issues related to the equivalent of Mockito's verifyNoMoreInteractions() (#6, #50) and I think there are still some use cases which current DSL does not fully cover.

I have the following test:

// There are interactions with mock2 before that like initialization,
// which are not important for the test
verifyOrder {
    mock1.firstCall()
    mock2.someCall()
    mock1.secondCall()
}
// There are interactions with mock1 afterwards 
// but there are no more interactions with mock2

How can I check that there were no more interactions with mock2?

I could probably add the following:

verifyAll {
    mock2.someCall()
}

but it doesn't work when there were calls to mock2 before someCall() (which are irrelevant for the current test, like some initialization calls etc.).
Even in cases where it would work this leads to duplicate code, which quickly gets ugly when there are many calls or methods have many parameters etc.

Would be nice if there was something like:

verify { mock2 wasNot CalledAnymore }
@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Dec 29, 2018

Is it possible to use clearMocks(answers = false, recordedCalls = true, childMocks = false) to remove only recorded calls before doing next interactions and use verify { mock wasNot called } to verify no more interactions. Looks not too ugly 😄 adding such new concepts as you propose require very detailed understanding of a problem. so if this solution I proposed satisfies you, I would just wait another similar case to make sure it is needed feature + understand more

@oleksiyp oleksiyp changed the title verifyNoMoreInteractions() Feature: memorizing checked calls and allow to verify no more interactions to make it similar to Mockito verifyNoMoreInteractions() Dec 29, 2018

@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

I think my use case is a little bit different. You wrote "remove only recorded calls before doing next interactions". But the point is, the test is written according to the "given - when - then" structure. This means: first there's the setup phase, then interaction with mocks (calling business logic methods), then verification of what has been called in the interaction phase and possibly assertions of the results. There are no more calls in/after the verification phase.

So all calls already happened at the time I'm calling verifyOrder. I only need to check if there were no more calls in the interaction phase to mock2 (after the calls listed inside verifyOrder). I hope it's clear. if not I can provide another example, just let me know.

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

Oh, now I think I understand what it means:

// There are interactions with mock1 afterwards
// but there are no more interactions with mock2

You mean that there are not-checked interactions, not the fact you do some interactions.

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

Please correct me if I am wrong.

You have following calls:

mock2.initCall()
mock1.firstCall()
mock2.someCall()
mock1.secondCall()

Then you do following verify checks:

verifyOrder {
    mock1.firstCall()
    mock2.someCall()
    mock1.secondCall()
}

But I don't get how you can do(if it would exists):

verify { mock2 wasNot CalledAnymore }

coz

mock2.initCall()

is still not checked.

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

The solution would be to verify this initCall and use some new construct to check.

verify { mock2.initCall() }
ackFullyVerified(mock1, mock2)

For me ackFullyVerified(or checkFullyVerified) sounds better than no more interactions or not called anymore

@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

Yes, but I don't care about the initCall in this case (it's not part of the test contract, it's not being verified).

Let's say I have following system under test:

class SystemUnderTest(val dependency1: Dependency1, val dependency2: Dependency2) {
    fun runBusinessLogic(variant: Boolean) {
        dependency1.init()
        dependency2.init()
        dependency1.callSomeMethod()
        dependency2.callSomeMethod()
        dependency1.callOtherMethod()
        if (variant) dependency2.callOtherMethod() // this is where I want to check 
                     // that the call didn't happen for variant == false
    }
}

And then I test it like this:

@Test
fun testMySystem() {
    // Given - initialization phase
    val dependency1 = mockk<Dependency1>(relaxed = true)
    val dependency2 = mockk<Dependency2>(relaxed = true)
    val systemUnderTest = SystemUnderTest(dependency1, dependency2)

    // When - running actual business logic
   systemUnderTest.runBusinessLogic(variant = false) // in the background, methods on dependency1 and dependency2 are called

    // Then - verification and assertion
    verifyOrder {
        // we don't care about the init() methods
        dependency1.callSomeMethod()
        dependency2.callSomeMethod()
        dependency1.callOtherMethod()
    }
    // Here I want to make sure that there were no other calls to dependency2 
    // when running the business logic
}
@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

The solution would be to verify this initCall and use some new construct to check.

verify { mock2.initCall() }
ackFullyVerified(mock1, mock2)

For me ackFullyVerified(or checkFullyVerified) sounds better than no more interactions or not called anymore

The problem is that in some testing scenarios e.g. equals() methods are being called on mocks etc. I don't want to include these equals() calls in the verification, because these calls are testing framework artifacts and not part of my business logic.

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

Got your point about equals/hashCode(this is kind of another topic to address)
Maybe I still do not understand it from point of view of the new construct.

Tell me how you would solve it in Mockito? I doubt you can do the same without

verify(mock2).initCall()
verifyNoMoreInteractions()
@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

One more proposal(not much connected to discussed use-case) is something like this:

ackAdditionallyCalled {
    mock2.initCall()
    mock2.equals(any())
}
ackFullyVerified(mock2)
@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

Actually it was working exactly like this in Mockito. I started having problem with the test logic after migrating to MockK.

@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

These are tests in ThirtyInch, an MVP framework.

Original test (Java & Mockito):

    @Test
    public void testRemoveOtherObserver() throws Exception {
        mPresenter.create();

        // add observers only for attach event
        final TiLifecycleObserver observer1 = mock(TiLifecycleObserver.class);
        mPresenter.addLifecycleObserver(observer1);
        final TiLifecycleObserver observer2 = mock(TiLifecycleObserver.class);
        final Removable removable = mPresenter.addLifecycleObserver(observer2);

        // when observer1 receives the first event it unregisters observer2
        doAnswer(new Answer() {
            @Override
            public Object answer(final InvocationOnMock invocation) throws Throwable {
                removable.remove();
                return null;
            }
        }).when(observer1).onChange(any(TiPresenter.State.class), anyBoolean());

        mPresenter.attachView(mock(TiView.class));

        final InOrder inOrder = inOrder(observer1, observer2);

        //observer 1 receives pre onAttachView event
        inOrder.verify(observer1).onChange(TiPresenter.State.VIEW_ATTACHED, false);

        // observer2 receives the pre event even when observer1 removed observer2 before observer2 received the pre event
        inOrder.verify(observer2).onChange(TiPresenter.State.VIEW_ATTACHED, false);

        // observer 1 receives post onAttachView event
        inOrder.verify(observer1).onChange(TiPresenter.State.VIEW_ATTACHED, true);

        // observer2 never receives the post event, is unregistered at that time
        verifyNoMoreInteractions(observer2);
    }
@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

The same test in Kotlin + MockK (it is working, as I included a workaround in the last line with exactly = 1):

    @Test
    fun `test remove other observer`() {
        presenter.create()

        // add observers only for attach event
        val observer1 = mockk<TiLifecycleObserver>(relaxUnitFun = true)
        presenter.addLifecycleObserver(observer1)
        val observer2 = mockk<TiLifecycleObserver>(relaxUnitFun = true)
        val removable = presenter.addLifecycleObserver(observer2)

        // when observer1 receives the first event it unregisters observer2
        every { observer1.onChange(any(), any()) } answers { removable.remove() }

        presenter.attachView(mockk())

        verifyOrder {
            //observer 1 receives pre onAttachView event
            observer1.onChange(TiPresenter.State.VIEW_ATTACHED, false)

            // observer2 receives the pre event even when observer1 removed observer2 before observer2 received the pre event
            observer2.onChange(TiPresenter.State.VIEW_ATTACHED, false)

            // observer 1 receives post onAttachView event
            observer1.onChange(TiPresenter.State.VIEW_ATTACHED, true)
        }
        // observer2 never receives the post event, is unregistered at that time
        verify(exactly = 1) { observer2.onChange(TiPresenter.State.VIEW_ATTACHED, any()) }
    }
@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

The last line in the Kotlin test is because I know exactly that observer2.onChange() method should not be called second time (like it was called on observer1).
But if I didn't know that and just wanted to make sure there were no more interactions at all afterwards with observer2 then I cannot do it in MockK.

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

Ok, I should admit communication is difficult 😄 (I mean in general).
Why you are not able to do the following thing:

        verifySequence {
            //observer 1 receives pre onAttachView event
            observer1.onChange(TiPresenter.State.VIEW_ATTACHED, false)

            // observer2 receives the pre event even when observer1 removed observer2 before observer2 received the pre event
            observer2.onChange(TiPresenter.State.VIEW_ATTACHED, false)

            // observer 1 receives post onAttachView event
            observer1.onChange(TiPresenter.State.VIEW_ATTACHED, true)
        }
@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

Because there are equals() calls to observer1 and observer2. I think they are caused by the answers logic:

every { observer1.onChange(any(), any()) } answers { removable.remove() }
@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

That is an interesting question why they are happening. Would be nice to understand it.

Anyway maybe you just need ta construct to exclude this calls from recording:

excludeRecording {
     observer1.equals(any()) 
     observer2.equals(any()) 
}

or shorter convenient form:

excludeRecording(equals = true, toString = true, hashCode = true) 
@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 1, 2019

This would probably also work, if it's easier to implement for you.

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

PR with changes: #212

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 1, 2019

In PR you can exclude calls that are not needed:

excludeRecords {
    mock.call(any(), eq("test"))
}

and finally acknowledge verification:

ackVerified(mock)

to make sure all other recorded calls are verified.

I think it is generic enough and useful for many people.

After all you can just excludeRecords + do verifySequence if needed.

@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 3, 2019

Just tested it and it works very well like this:

val observer2 = mockk<TiLifecycleObserver>(relaxUnitFun = true)
excludeRecords { observer2 == any() }

and then verification

ackVerified { observer2 }

However I find the name ackVerified a little bit cryptic.
Maybe it would be possible to overload verifyAll to take a mock as a parameter? Then we could write:

verifyAll(mock)
@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 3, 2019

On second thought, maybe verifyAll would be misleading in this case. Maybe confirmAllVerified or something like this?

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 3, 2019

confirmAllVerified is indeed better description, but quite long. Ill try to play with it more

@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 3, 2019

Maybe then simply confirmVerified?

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 3, 2019

Yes this one is really good candidate. Just give me some time to put my hands on the code to decide

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 3, 2019

Changed to confirmVerified + added possibility to clear verification marks and exclusion rules

@oleksiyp oleksiyp added the fixed label Jan 4, 2019

@oleksiyp

This comment has been minimized.

Copy link
Collaborator

oleksiyp commented Jan 4, 2019

Versions 1.9 or version 1.9.kotlin12 were released with this change included. Please check it and close the ticket if everything is OK.

@lukaszkalnik

This comment has been minimized.

Copy link

lukaszkalnik commented Jan 4, 2019

Wow, that was fast! This is an outstanding support, thank you so much 👍

Tested 1.9, works great 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment