In order verification counts out of order invocations

[GoogleCodeExporter]

What steps will reproduce the problem?

    @Test
    public void inOrderTest() throws IOException{
        final String message = "Hello World!\n";

        final Writer mockWriter = mock( Writer.class );

        for( char c : message.toCharArray() )
            mockWriter.write( c );

        final InOrder inOrder = inOrder( mockWriter );
        for( char c : message.toCharArray() )
            inOrder.verify( mockWriter ).write( c );
        inOrder.verifyNoMoreInteractions();
    }

What is the expected output? What do you see instead?

This test should pass, however it fails with the message 

Verification in order failure:
mockWriter.write(108);
Wanted 1 time:
-> at 
org.bitbucket.artbugorski.brainfuj.interpreter.InterpreterTest.inOrderTest(Inter
preterTest.java:62)
But was 3 times. Undesired invocation:
-> at 
org.bitbucket.artbugorski.brainfuj.interpreter.InterpreterTest.inOrderTest(Inter
preterTest.java:58)

What version of the product are you using? On what operating system?

Mockito 1.8.5

java version "1.6.0_23"
OpenJDK Runtime Environment (IcedTea6 1.11pre) (6b23~pre11-0ubuntu1.11.10)
OpenJDK 64-Bit Server VM (build 20.0-b11, mixed mode)

Linux 3.0.0-13-generic #22-Ubuntu SMP x86_64 x86_64 x86_64 GNU/Linux

Please provide any additional information below.

Originally asked as question here: 
http://stackoverflow.com/questions/8372884/how-to-test-in-mockito-for-specific-o
rder-of-calls-with-same-arguments

Original issue reported on code.google.com by Art.Bugo...@gmail.com on 4 Dec 2011 at 6:51

[GoogleCodeExporter]

I've just posted a work-around on the Stack Overflow thread.  My opinion is 
that this isn't actually a bug, but still a gotcha.

This is a specific example of a more general issue; that is, that the inOrder 
features don't give a way of verifying that a mock has had (for example) one 
call to methodA, then one call to methodB, then another call to methodA.  
Perhaps what we need is a VerificationMode that could be used in combination 
with inOrder, that means "this many times for now, but possibly more times 
later".  So we could write something like

inOrder.verify( theMock, timesForNow( 1 )).methodA();
inOrder.verify( theMock, timesForNow( 1 )).methodB();
inOrder.verify( theMock, timesForNow( 1 )).methodA();

This would solve the original issue.  I will have a go at writing a 
VerificationMode that does this.  If I succeed, I'll contribute a patch.  
Unless anyone has any better ideas for a solution for this.

Cheers,
David.

Original comment by dmwallace.nz on 5 Dec 2011 at 7:45

[GoogleCodeExporter]

I suppose that depends on your definition of "bug". I suspect the code is 
correct, but it definitely doesn't do what is expected so I'd say it's a user 
experience bug.

Original comment by Art.Bugo...@gmail.com on 5 Dec 2011 at 2:33

[GoogleCodeExporter]

I've done some more investigation.  There's definitely a bug here.  The 
behaviour is absolutely not consistent.  I'm going to try to track down the 
bug, and fix it if I can, but the Mockito code in this area is not the easiest 
to understand.

It appears that Art's test fails when the String being processed has a double 
letter in it.  If you change "Hello World" to "Bonjour le Monde", the test 
passes, even though the letter "o" appears three times in this String.  I'm 
guessing it's the double L in "Hello" that's causing the error, but instead of 
being reported as "l" occurring twice, it's reported as occurring 3 times, 
because of the third "l" that occurs later in the String. 

Original comment by dmwallace.nz on 7 Dec 2011 at 7:13

[GoogleCodeExporter]

I don't think that the behaviour here can be made consistent, without the 
possibility of breaking somebody's existing tests.  The method 
getFirstMatchingChunk in InvocationsFinder finds the next sequence of 
consecutive matches of the verify, among all the invocations that the InOrder 
knows about.  This is then checked by the VerificationMode, which in this case 
is times(1).  

In Art's example, during the first call to 
inOrder.verify(mockWriter).write('l')
the return value from getFirstMatchingChunk is the two consecutive invocations 
of mockWriter.write('l').  Since there are two invocations here, it doesn't fit 
with times(1), and the test fails.

Maybe what is required is a verification mode that matches just one invocation, 
but doesn't fail when there are two invocations.  So we could write something 
like
inOrder.verify(mockWriter,oneCall()).write(c)
to achieve the desired result.  The default behaviour of inOrder verification 
would still be inconsistent, but at least there'd be a workaround, and nobody 
would have their existing test broken.

At the same time, it would be good to change the way the error is reported, so 
that it's clearer what's going on.  It seems wrong to report (in this case) 
that the test failed because there were 3 matching invocations; the truth is 
that the test failed because there were 2 consecutive matching invocations.

Does anybody in the Mockito team have any feedback on this?  Once more, I am 
volunteering to provide the required patch; but I wanted to check that I'm 
doing the right thing, before I begin work on this.

Cheers,
David.

Original comment by dmwallace.nz on 7 Dec 2011 at 8:10

[GoogleCodeExporter]

Hey David, thanks a lot for all the effort!

Check out this wiki: 
http://code.google.com/p/mockito/wiki/GreedyAlgorithmOfVerficationInOrder

I'm hoping it sheds some light why in-order verification works in 'chunks' of 
interactions. The bottom line is that regular verification works in 'chunks' so 
it made sense to keep things consistent. It might have been a wrong decision to 
start with but you never know :) I'm pretty sure we would find other gotchas if 
we designed it differently. 

I'm open to discussion how we can improve it. The error message could be 
better. Though it will be tricky to implement. It seems to me that in-order 
verification is not greatly suitable for current Mockito API. The bottom line 
is that at the point of a single inOrder.verify(...) it is hard to make a 
validation because it depends on the subsequent declarations.

Consider this:

a
bb
c
b
d

Valid:
verify(b, times(2))
verify(c)

Also valid:
verify(b, times(3))
verify(d)

But this is not valid:
verify(b, times(3))
verify(c)

So it's hard to make it consistent really. Sometimes verify(b, times(2)) is 
valid and sometimes verify(b, times(3)) is valid.

In theory, the only general solution to the problem might be investing in a 
different way of verifying. E.g. some api that lets me first declare the 
desired order, and then verify it at the end. For example:

inOrder.interaction(mock).foo();
inOrder.interaction(mock).bar();
inOrder.verifyAll()

Original comment by szcze...@gmail.com on 7 Dec 2011 at 10:19

[GoogleCodeExporter]

Szczepan, thanks for posting the wiki link.  I understand the arguments on the 
wiki, and the reason for the "greedy in sequence" behaviour of inOrder/times.  
I don't quite agree with your argument about consistency above; I think it 
would be perfectly possible to make a consistent, non-greedy version of 
inOrder/times.  However, it would behave more like atLeast() than times(), 
because if there were extra unmatched invocations, it wouldn't fail.

Since it wouldn't be too much like the non-inOrder version of times(), I think 
it should be called something different.  That's what I suggested in comment 
#4.  It means that we don't risk breaking anyone's existing tests, and also 
that people have the choice of whether to get the non-greedy or the "greedy in 
sequence" behaviour.  

I would like to have a go at implementing this.  Either I'll succeed (in which 
case I'll give you a patch); or I'll gain a much better understanding of why 
it's not possible to do this (in which case I'll add some helpful comments to 
the wiki page).  I think that inventing a whole new means of verification is 
probably overkill for this problem.

Cheers,
David.

Original comment by dmwallace.nz on 8 Dec 2011 at 7:49

[GoogleCodeExporter]

>However, it would behave more like atLeast() than times(), because if there 
were extra unmatched invocations, it wouldn't fail.

I guess that's why my mind treats it as 'inconsistent'. E.g. the regular 
verification (without using any verification mode like times(x) or atLeast()) 
does require exact number of calls.

Sure take a stab at the implementation.

Cheers!

Original comment by szcze...@gmail.com on 8 Dec 2011 at 9:59

[GoogleCodeExporter]

Hi David, I personnaly would like to see such code.

Original comment by brice.du...@gmail.com on 8 Dec 2011 at 10:01

[GoogleCodeExporter]

Just assigned you this issue.

Original comment by brice.du...@gmail.com on 13 Jan 2012 at 10:24

[GoogleCodeExporter]

Original comment by brice.du...@gmail.com on 13 Jan 2012 at 10:24

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

I have merged a change back to the main branch, to provide a verification mode 
as per my comment number 1 here.  Instead of timesForNow( n ), it's calls( n ). 
 This will be included in the next release of Mockito.  The issue about the 
error being reported incorrectly has not been dealt with yet.

Original comment by dmwallace.nz on 31 Jan 2012 at 7:49

[GoogleCodeExporter]

Original comment by brice.du...@gmail.com on 5 Mar 2012 at 12:24

• Added labels: Priority-High
• Removed labels: Priority-Medium