Ayende @ Rahien

Refunds available at head office

Request for comments: Changing the way dynamic mocks behave in Rhino Mocks

I have just committed a change to the way Rhino Mocks handles expectations for dynamic mocks and stubs.  Previously, the meaning of this statement was "expect Foo() to be called once and return 1 when it does":

Expect.Call( bar.Foo ).Return(1);

Now, the meaning of this is: "expect Foo() to be called one or more times, and return 1 when it does". This means that this will work:

Assert.AreEqual(1, bar.Foo);
Assert.AreEqual(1, bar.Foo);
Assert.AreEqual(1, bar.Foo);

Where as previously, using dynamic mocks, it would fail on the second assert, because the expectation that was setup was consumed. I think that this is a more natural way to behave, but this is a subtle breaking change.
You can get the old behavior by specifying .Repeat.Once().

Thoughts?

Comments

Ryan Kelley
10/10/2008 11:56 AM by
Ryan Kelley

I am still fairly new to RhinoMocks but a question about this. It seems more logicical to me when you tell your mock to expect a call on a property/method and then give it a value to return. So do you still set it up with Expect.Call and then assert the values later or only do the assertion? If that is the case where do you setup what the mock is supposed to return to an object that is calling it?

Demis
10/10/2008 12:53 PM by
Demis

I think for dynamic mock / stubs this looks like it should be the expected behaviour. Strict Mocks should remain explicit.

Darrell Mozingo
10/10/2008 12:55 PM by
Darrell Mozingo

I like it.

How would it be a breaking change, though? It's a dynamic mock in the first place, so even if people specified the same expectation two or more times to satisfy the previous behavior, their test would still pass, no? Their extra expectations wouldn't be needed any longer, but it wouldn't hurt to have them there, either, especially if they're all returning the same values, expecting the same parameters, etc.

Unless they relied on the fact that the expectation could only be consumed once for their test to pass, but that seems like a smell and unnatural anyway. It would be a good incentive to fix those tests :)

Jason Meckley
10/10/2008 01:01 PM by
Jason Meckley

I'm torn. On the one hand the syntax is cleaner/simpler. on the other hand explicitly requiring 3 calls (with an expectation to be called 3 times) is more explicit, albeit more fragile.

Demis makes a good point that the new behavior should only apply to dynamic mocks, not strict mocks.

enough thinking out loud... I like it.

Ayende Rahien
10/10/2008 01:19 PM by
Ayende Rahien

Jason,

It is applicable to stubs and dynamics, not to strict

KG
10/10/2008 01:27 PM by
KG

When I first saw this syntax I definitely thought that I was defining a rule (return 1 whenever this action occurs) rather than an explicit action (the next time in the recording phase that this action occurs, return 1).

That being said, I'm not sure that the syntax of

Expect.Call( bar.Foo ).Return(1);

actually indicates which behavior should be expected. Repeat.Once definitely does.

Ray
10/10/2008 01:40 PM by
Ray

I like this change and it has been my main gripe with RM, (although in retrospect I was probably not using the right kind of mock- shame on me)

Stephen
10/10/2008 01:47 PM by
Stephen

I like it..

Chris Holmes
10/10/2008 02:47 PM by
Chris Holmes

I agree. I've always wished that Dynamic Mocks would work this way. Now they will.

Bahador
10/10/2008 03:00 PM by
Bahador

It is really more natural this way.

Now it is less "surprising" !

Steve Bohlen
10/10/2008 03:04 PM by
Steve Bohlen

I am definitely in favor of this change and agree with others who have said that this 'behavior' seems more 'natural' when defining a dynamic mock/stub than the current behavior.

In such a 'dynamic-XXX' context, the paradigm is sort of already 'let me say what I want to happen and if something else happens, just disregard it please'. In a sense we are really saying that the repeated call is something 'else' even though its technically the same as the first thing I was explicit about.

The implicit .Repeat().Any() that you are suggesting here that I can later get more specific with a .Repeat().Once() if I want to makes much more sense to me in the dynamic context.

Also +1 for leaving strict-XXX behavior...strict!

Chris Brandsma
10/10/2008 03:20 PM by
Chris Brandsma

I like the change. That is the behavior I want most of the time anyway.

Mike
10/10/2008 04:27 PM by
Mike

+1...seems to fall more in line with the intent of a stub.

Calvin Bottoms
10/10/2008 06:08 PM by
Calvin Bottoms

I like it. With the old behavior, a replace-temp-with-query refactoring could easily break a test. I was often having to go back and explicitly indicate one or more calls after having broken a test this way. This is much better. Thank you.

chrissie1
10/10/2008 06:32 PM by
chrissie1

I'm not sure if this is better. The other way made you think more about your code. Like why did it do that twice? But I guess I will just have to be more explicit. And it is all yours anyway. And as long as it works in VB ;-)

Benny Thomas
10/10/2008 10:08 PM by
Benny Thomas

Expect.Call( bar.Foo ).Return(1).EveryTime();

Had been better in my opion.

Jeremy Gray
10/10/2008 10:21 PM by
Jeremy Gray

Upgrading from 3.5 RC1 to 3.5 RTM is already going to break dozens of my tests, so what's a few more. ;) Not that I'm happy about there being breaking changes between a release candidate and an RTM, though. :(

In all seriousness, I don't think I want mocks changing to default to allowing multiple calls. If my code says "expect a call", I expect a call. I'll add repeat if I want it. I doubly-dislike that this would be a breaking change to a very highly used piece of Rhino.Mocks. :(

Now, for stubs, on the other hand, defaulting to the equivalent of Repeat.Any() makes perfect sense.

Jeremy Gray
10/10/2008 10:25 PM by
Jeremy Gray

Addendum - I'll have to give more thought to the fact that we are talking about dynamic mocks here, not "classic" ones, but I suspect my opinion will probably still hold since dynamic mocks are the basis of the AAA mocks, no? If so, then I'm still against the change.

Ernst
10/11/2008 11:11 AM by
Ernst

I like the change, I've been bit a couple of time by the previous behavior.

Hendry Luk
10/13/2008 07:29 AM by
Hendry Luk

I actually prefer explicit distinction between expectation and stubbing. I.e., Expect.Call() IMO would be more natural to expect exactly 1 call; whereas Stub.Call() would merely stub a return value regardless of number of calls (including zero). This is also the same semantics used in many mockers e.g. jMock.

Nonetheless, I hope AAA on dynamic mock would still expect 1 call by default?

Hendry Luk
10/13/2008 07:35 AM by
Hendry Luk

*Stub.Call()

Or rather SetupResult.For() in Rhino.Mocks

Peter Morris
10/13/2008 07:44 AM by
Peter Morris

Repeat.Once is unclear. Repeat is repetition so therefore

"Perform once" means to perform an action only once, whereas "Repeat once" means to perform an action twice (one original time, and one repeated time).

Because of this I have always read Repeat.Once to mean 1+1 and not 1. I have always thought "Perform" would be more clear than "Repeat"

Perform().Once();

Perform().Times(3);

Perform().ManyTimes();

Back onto your subject.

To me Expect.Call() means two things

01: It is mandatory

02: It is singular

This is because you use the word Call and not the plural Calls. Whereas SetupResult.For() has no implications of how many times and is therefore easily assumed to mean "as many times as you need".

When testing I either need to set up a result for a method call (SetupResult) or test that something mandatory happens (Expect.Call). When I expect a call 99% of the time I expect it to be called only once, and the current scheme implies that is what I am doing. Therefore having to add Repeat().Once() is bad in two ways

01: 99% of the time I have to type more.

02: Due to the wording it implies 2 calls minimum, and is confusing for someone who is new.

I think it is always better to break code at compile time rather than execution time. With that in mind I would recommend changing the naming to something like

Expect.OneCall( a.B() ).Return(1);

and

Expect.CallsTo (a.B() ).Return(1);

If you implement "CallsTo" then it is important to rename Call because I think having "Call" and "CallsTo" are too similar.

maybe you should also rename Repeat to Exactly

Expect.CallsTo( ... ).Exactly(2).Return(1);

To summarise

Expect.OneCall(...).Return(1); - 1..1 calls (no Repeat option available)

Expect.CallsTo(...).Return(1); = 1..* calls

Expect.CallsTo(...).Return(1).Exactly(5); 5..5 calls

I hope this posts okay :-)

Comments have been closed on this topic.