Bug in .Repeat.Times(x) ??

[andreister]

The previous post "Assert # of times a method was called" brings me to
the following scenario

==================================================
[Test]
public void Test()
{
var foo = MockRepository.GenerateMock<IFoo>();
foo.Expect(x => x.Bar()).Repeat.Times(5);

Boo.Run(foo, 4);

foo.VerifyAllExpectations();
}

public class Boo
{
public static void Run(IFoo foo, int total)
{
for (int i = 0; i < total; i++) { foo.Bar(); }
}
}

public interface IFoo
{
void Bar();
}
==================================================

Obviously, it fails with "Expected #5, Actual #4."

However, if we change ".Repeat.Times(5);" to ".Repeat.Times(2);" it
passes!!? (I would expect a failure with "Expected #2, Actual #4.")


It looks like "as designed" behavior, since
UnorderedMethodRecorder.DoGetRecordedExpectationOrNull (https://rhino-
tools.svn.sourceforge.net/svnroot/rhino-tools/trunk/rhino-mocks/
Rhino.Mocks/MethodRecorders/UnorderedMethodRecorder.cs) relies on
"triplet.Expectation.CanAcceptCalls" that is NOT updated for EVERY
call... But it's quite confusing.

[ssteinegger]

MockRepository.GenerateMock creates a dynamic mock, which allows calls
that weren't expected. To do this I think you'll need a strict mock
which cannot be created with the static repository.

[andreister]

Yes, but VerifyAllExpectations should address that some method was
called *more* than expected.

Otherwise Times(x) should have been called "AtLeast(x)" !

[ssteinegger]

You're right, I didn't say how it _should_ be, just how it probably
_is_.
But I could be wrong and it's actually a bug.

[Tim Barcz]

I'll toss in my two cents....

If it's a strict mock, should throw an exception....
If it's a dynamic mock this is expected.

If we start treating the syntax different between strict and dynamic mocks I think the learning curve goes up.  Right now the differences in behavior lie within which mock object you use and NOT the syntax you use on the mock, which is how I personally prefer it.

Tim

[ssteinegger]

This also means, that Repeat.Times never makes sense on a dynamic
mock, because it is the same as Repeat.AtLeast (but doesn't say this).
Independent of the syntax, this is not so nice. Repeat.Times (or Once
or Never) should always be kind of strict.

On 13 Feb., 15:28, Tim Barcz <timba...@gmail.com> wrote:
> I'll toss in my two cents....
>
> If it's a strict mock, should throw an exception....
> If it's a dynamic mock this is expected.
>
> If we start treating the syntax different between strict and dynamic mocks I
> think the learning curve goes up.  Right now the differences in behavior lie
> within which mock object you use and NOT the syntax you use on the mock,
> which is how I personally prefer it.
>
> Tim
>

[Shane C]

Agreed. This really does not seem like correct behavior. So who has
time to create & send Ayende a patch? :D

[Ayende Rahien]

I think that you just vulanteered

[Shane Courtrille]

Haha I know I did.. I was just hoping someone else would have the time
so I can spend a little bit of my time with my family.. if not.. then
I shall follow the Ayende method of "Fix the things that bug you"

[Shane C]

I've tracked down the code that causes this behavior and it's pretty
deep in the system. ReplayDynamicMockState is the one making the
decision to ignore the extra call and just return the default value
but the problem is that it relies on
MethodRecordBase.GetRecordedExpectationOrNull to return Null for this
behavior.

The safest place to make a change would appear to be
ReplayDynamicMockState but this doesn't work because it needs an
expectation so it can tell it to return or throw. The problem being
that we don't an expectation since the return of a null expectation is
what triggers this behavior. There appears to be a lot of other code
that all relies on GetRecordedExpectationOrNull so changing it's
behavior seems like an unsafe idea but I don't see how the problem can
be fixed without doing so.

Look at the else statement in ReplayDynamicMockState.DoMethodCall to
get a better idea of what I mean...

Thoughts?

On Feb 17, 1:07 pm, Shane Courtrille <shanecourtri...@gmail.com>
wrote:

> Haha I know I did.. I was just hoping someone else would have the time
> so I can spend a little bit of my time with my family.. if not.. then
> I shall follow the Ayende method of "Fix the things that bug you"
>
> On Tue, Feb 17, 2009 at 12:56 PM, Ayende Rahien <aye...@ayende.com> wrote:
> > I think that you just vulanteered
>

[Ayende Rahien]

Can you create a failing test, I didn't follow this thread too closely

[Ayende Rahien]

Got the failing test, but I would say that this is not a bug, it is a by design feature.

DynamicMock sole reason for being is that it accepts unexpected calls.

If you want this to work, you need to use StrictMock

[Shane Courtrille]

The problem I have with StrictMock is that it seriously breaks the "Assert one thing per test" practice.  I have to have an expectation for every single call that occurs against that interface.  As soon as I have an interface I make different calls on I am out of luck if I want to follow this practice.  I prefer to break things up by using a dynamic mock.  The problem is now that I can't also verify that the mock was called the number of times I expect. 

Another problem I have is with consistency of interface.  DynamicMocks should probably have a seperate interface that only has Repeat.AtLeast() instead of Times since you can't actually trust the Times.

[Ayende Rahien]

You probably want to go the other way, and assert that it was only called once, that would be much easier.

[Shane Courtrille]

That assumes I only call a method once.  As soon as I have code that needs to loop over something and do something with each value it breaks down.  I can setup my test so that I only have one loop iteration and then make sure that it only occurs once but this still doesn't change the fact that .Times() can lie on DynamicMocks since it's always AtLeast()

[Ayende Rahien]

You can change the expectation with dynamic mocks, so it would know about that.

[Shane Courtrille]

Sorry I'm not sure what you mean "about that" ? 

[Ayende Rahien]

so the expectation can tell that it is in dynamic mock, and respond appropriately

[Stefan Steinegger]

Had another idea.

Times still makes sense even with a dynamic mock. You aren't only
defining expectation, you also define behaviour. What you actuall say
with "Times(5)" is "the first 5 times it is called". After that, it is
not defined (meaning "return null" for a dynamic mock or "throw and
exception" for a strict mock).

You could do this:

var foo = MockRepository.GenerateMock<IFoo>();

foo.Stub(x => x.Bar()).Repeat.Times(5);
foo.Stub(x => x.Bar()).Throw(new InvalidOperationException
("Gotcha!"));

Boo.Run(foo, 6);

PS: didn't try it, just a guess.

On Feb 23, 4:56 am, Ayende Rahien <aye...@ayende.com> wrote:
> so the expectation can tell that it is in dynamic mock, and respond
> appropriately
>

> On Sun, Feb 22, 2009 at 9:24 PM, Shane Courtrille <shanecourtri...@gmail.com

> >>>>>> by design feature.DynamicMock sole reason for being is that it

> >>>>>> accepts unexpected calls.
> >>>>>> If you want this to work, you need to use StrictMock
>
> >>>>>> On Sun, Feb 22, 2009 at 12:34 PM, Ayende Rahien <aye...@ayende.com>wrote:
>
> >>>>>>> Can you create a failing test, I didn't follow this thread too
> >>>>>>> closely
>

> >>>>>>> On Sun, Feb 22, 2009 at 12:01 PM, Shane C <shanecourtri...@gmail.com

[Ayende Rahien]

Yep, that should work, and probably the best suggestion

[Shane Courtrille]

Actually if you do Times you get the default value after that many times... which is actually kind of strange if you think about it.