Skip to content
This repository

Use the new expectation syntax for mocks #153

Closed
iain opened this Issue June 15, 2012 · 128 comments
Iain Hecker
iain commented June 15, 2012

If you want to use the new expectation syntax, then the current mock syntax will look out of place.

That's why I propose we introduce the following matcher:

expect(object).to receive(:message).with(arg1, arg2)

Some considerations:

  • This would not be usable without rspec-expectations. I don't know how problematic this is, but it is a feature of rspec-mocks.
  • I don't know if we should change stub. It's not an expectation/assertion, so I don't think it needs to change.
Myron Marston
Owner

This would help solve #116, as it's essentially the same delegate issue we've now dealt with in rspec-expectations.

This would not be usable without rspec-expectations. I don't know how problematic this is, but it is a feature of rspec-mocks.

I can think of some ways to make this work w/o depending on rspec-expectations using feature detection.

Justin Ko

Basically, RR[1] provides the cleanest solution to this issue. I would like to see something like this in rspec-mocks:

stub(User).all { the_users } # uses `method_missing`
stub(User, :all) { the_users } # does not use `method_missing`

mock(User).all # an "expectation" that `all` will be called

double('the user') # same as the current `double`

To utilize stub and mock as in the example above, it would cause a massive breaking change. Totally worth it IMO.

/cc @dchelimsky

1.) https://github.com/btakita/rr

Myron Marston
Owner

I thought about this a bit more, and I came up an idea that's similar, but avoids the massive breakage:

stubbing(User, :all) do
  the_users
end

mocking(User, :all)

The "-ing" forms aren't taken up by anything yet, and read nicely when used with the block form. It looks a little weird when you're just mocking it w/o an implementation, though.

The feedback on the recent syntax changes in rspec-expectations have been overwhelmingly positive, and I think the fact that we didn't break anything is a big part of that. I don't think massive breaking change is acceptable at this point, unless we decide to do it in a separate gem (e.g. rspec-better-mocks or whatever).

On a side note, if we can come up with a good solution here, we have the potential to get rspec in a place where the monkey patching it does is very, very minimal. Up to now, RSpec has added the following methods to every object in the system:

  • describe
  • should
  • should_not
  • should_receive
  • should_not_receive
  • stub
  • stub!
  • and maybe some others I'm not thinking of.

should and should_not have been dealt with w/ the expect syntax, and I just fixed rspec core in rspec/rspec-core#638 to no longer add describe to every object. The methods added by rspec-mocks are the last major source of monkey patching ever object, so if we can find a good solution here it would be great :).

David Chelimsky
Owner

The "ing" idiom suggests to me "scoped within this block" e.g.

stubbing(User, :all => [User.new]) do
  # stub is scoped to this block
end
# that stub is no longer available

I actually have a never-used-lib that uses this syntax for that purpose.

Also, I'd prefer to have a syntax that aligns with the changes to rspec-expectations if we're going to do this. I'd be OK with:

stub(object, :method => value)
stub(object, :method) { lazy_value }
expect(object).to receive(:message)

I would not be OK with:

stub(object).method { lazy_value }
mock(object).method { lazy_value }

This syntax is no more terse than the others, but it is not intention revealing IMO.

Myron Marston
Owner

@dchelimsky -- I like your thinking here, but won't stub(object, :method => value) be a breaking change? Currently the stub method within an example creates a new test double; here it stubs a method on the given object.

This wouldn't just be a breaking change; this would be an incredibly confusing breaking change. Removing a method, so that users start getting NoMethodErrors is one thing; taking an existing method, and completely changing it's semantics so that it does something entirely different will be far more difficult for users to deal with. They'll get confusing failures and won't have an easy way to scan their code to find the old uses of stub.

David Chelimsky
Owner

@myronmarston STOP MAKING SENSE! :)

Yes, we'll need a new name, but that's the form I want to use: some_new_name(object, :method => value).

One option would be allow (comes from one of the java mock frameworks - can't recall which and google is not my friend today).

Myron Marston
Owner

Maybe stub_method(object, :method => value)?

Iain Hecker
iain commented June 20, 2012

Is it necessary to change stub at the same time? I know we want to fix the bug for stubbing, but should mocks wait for this?

@myronmarston For some reason I dislike the word method in there. I like the fact that mocks use the vocabulary of sending and receiving messages. Not sure on an alternative though.

David Chelimsky
Owner

Any objections to allow? Then we'd have:

allow(object, :method => value)
allow(object, :method) { lazy_value }
expect(object).to receive(:message)
Iain Hecker
iain commented June 20, 2012

Looks great!

Myron Marston
Owner

Any objections to allow?

A couple concerns I have:

  • allow makes sense on a pure mock object, because you'll get errors unless you mock or allow every method called on the object. But it makes very little sense on real objects. You're always allowed to call every method on a real object. When you're stubbing a method on a real object in order to force it's return value, allow doesn't really fit.
  • It seems like a shard departure from the current terminology employed by rspec-mocks. But maybe that's a good thing.

Here's another idea: respond_to:

respond_to(object, :method => value)
respond_to(object, :method) { lazy_value }
respond_to(object, :method).with(some_argument) { lazy_value }

When you stub a method, you're setting how an object responds to a particular message.

David Chelimsky
Owner

Unfortunately, respond_to(object, :method).with(:foo) sounds like "respond to object.method by returning foo"

Myron Marston
Owner

Yeah, I just realized that. API design is hard :(.

Myron Marston myronmarston closed this June 20, 2012
David Chelimsky
Owner

More challenging when you have legacy to support :)

Myron Marston myronmarston reopened this June 20, 2012
Dmytrii Nagirniak
dnagir commented June 20, 2012

Guys, how about this small idea...

Currently I write a lot of things like:

it "calculates thing weekly" do
  Calculator.should_receive(:annual_revenue).with(year: 5).and_return 520
  report.weekly_revenue.should == 10 # 520/52
end

The should_receive syntax is just a bit harder to read and type than what my eye & fingers want to:

it "calculates thing weekly" do
  Calculator.should_receive.annual_revenue(year: 5) { 520 }
  report.weekly_revenue.should == 10 # 520/52
end

Please consider this syntax or similar if it is something you think aligns with RSpec philosophy.

David Chelimsky
Owner

@dnagir that's an interesting idea, but chaining the target method onto should_receive is too different from everything else we do in RSpec IMO. My guess is that what you're looking for is something like RR's syntax, which lets you use the same syntax for the method once you wrap the object: mock(Calculator).annual_revenue(year: 5) { 520 }. That's very nice in terms of mirroring part of the actual use, but I think it's also a bit disconnected due to the mix of wrapping the object. I don't have a better idea at this point, but I'm not in favor of should_receive.annual_revenue (though I am personally in favor of receiving annual revenue).

Rodrigo Rosenfeld Rosas

@dchelimsky how about fake instead of stub?

Rodrigo Rosenfeld Rosas

Or pretend?

David Chelimsky
Owner

fake already has special meaning: http://xunitpatterns.com/Fake%20Object.html

pretend is interesting, but I don't like it initially - gut feeling. Need to think about it some more.

Myron Marston
Owner

Another possibility: stub_message--it's a bit like my earlier suggestion (stub_method) but retains the vocabulary of sending and receiving messages.

David Chelimsky
Owner

@myronmarston the message is what comes in, the method is how it responds.

Myron Marston
Owner

on_receipt_of(object, message).with(arguments)?

Actually, I was just thinking about the original allow idea some more. allow would allow us (pardon the pun) to do:

expect(object).to receive(:message)
allow(object).to receive(:message)

I'm still concerned about the potential confusion of the language of "allowing" a real object to receive a message it can already receive anyway, but the symmetry here is really, really nice and I might actually like this one best after all since none of the other ideas have that symmetry and we haven't yet come up with one we're happy with.

David Chelimsky
Owner

@myronmarston agreed so far, but open to other suggestions that maintain the same symmetry without the confusion about what allow means in the context of a real object.

Andreas Finger

I love the idea to use a consistent syntax for the mocks. And giving the stubs a different name, once the syntax changes, makes sense too. But allow does not sound that natural to me.

Maybe something like adjust or augment could work.

   expect(object).to receive(:message)
   augment(object).to receive(:message)

When staying close to the should_receive syntax, I would prefer something like 'simulate' or 'implements':

   Object.simulate(:method).with(param).and_return xyz
   Object.implements(:method).with(param).and_return xyz
Alexander Kern
kern commented July 12, 2012

What about implement rather than allow? It makes sense for real objects.

implement(object, :method) { :bar }
Myron Marston
Owner

I like the sense given by implement, but it doesn't read well as implement(object).to receive(:message), and I'd really like to have the symmetry of the same basic syntax.

augment (suggested by @mediafinger above) is pretty good, I think. It's certainly better for the case of a real object than allow, although I think for a pure mock object, allow still reads better.

So here's an idea: we can provide all 3 of these:

# sets a mock expectation
expect(object).to receive(:message)

# stubs the method
allow(object).to receive(:message)
augment(object).to receive(:message)

Basically, one of allow and augment would be an alias for the other, allowing the user to use whichever makes the most sense in their context (typically, augment for real objects and allow for pure mock objects).

David Chelimsky
Owner

We're not augmenting it to receive a message, but to allow it to receive the message. The more I think of this I like allow, and I definitely don't want to add two names for a new feature. FWIW, I thought of "allow" because JMock uses "allowing", so it comes from the mock-library space (just not Ruby).

Myron Marston
Owner

We're not augmenting it to receive a message, but to allow it to receive the message. The more I think of this I like allow, and I definitely don't want to add two names for a new feature. FWIW, I thought of "allow" because JMock uses "allowing", so it comes from the mock-library space (just not Ruby).

That's certainly true for a pure mock object (which is why I like allow a lot for that case), but IMO allow only makes sense if the object cannot already respond to the message--and with a real object that may not be the case. augment seems like a better word for real objects to me, but even it doesn't really have the right connotation.

David Chelimsky
Owner

I see what you're saying about real objects, but "augment this object to receive message x" doesn't make any sense based on similar arguments: it can already receive x. I don't have a good answer, but here are some more bad answers to see if any spark better ideas:

tell(object).to respond_to(:message).with(value)
redef(object, :message) { value }
replace(object, :message) { value }

I hate all three :)

Myron Marston
Owner

I've been thinking recently about the design benefits of pure mock objects. Partial mocks/stubs (i.e. on real objects) don't provide the same design and isolation benefits.

So...I think it makes sense to optimize rspec-mocks for the pure mock case while allowing its use on real objects. allow does this nicely: it reads really, really well for pure mock objects, and a bit funny for real objects. It's a subtle nudge that it's generally better to design your system and tests to work with pure mock objects.

All that is to say: I dislike your 3 suggestions as well, and I'm getting on board with adding expect/allow for mocks/stubs in spite of the funny wording for real objects.

Alexander Kern
kern commented July 12, 2012

+1 for allow. I like that it's borrowed from jMock, so it isn't Ruby-specific vocabulary. I agree with @myronmarston that making partial mocking ugly isn't necessarily bad. It should be discouraged.

Daniel Harrington
rubiii commented July 13, 2012

the explanation given by @myronmarston makes sense to me! +1

John Wilger

I'm also not a big fan of #allow, especially for stubbing methods on real objects (as opposed to pure stub/mock objects). How about #stub_on. I like the way that would read: stub_on( my_object, some_method: 'blah' ).

David Chelimsky
Owner

I think I could live with stub_on as long as we also use expect_on so the methods align. WDYT?

Myron Marston
Owner

stub_on and expect_on align nicely. But does it work with the rest of the fluent interface (e.g. with(some, args), exactly(3).times, etc? I'd like to preserve as much of that fluent interface as possible.

David Chelimsky
Owner

Again w/ the sense. That won't work at all, and I'm at a bit of a loss for new ideas. So far, I think expect/allow is the best pairing. More ideas?

John Wilger

Why does that not work at all?

stub_on( my_object, foo: 'bar' )

expect_on( my_object, :foo ).with( 'some', 'args' ).exactly( 3 ).times
# or
expect_on( my_object ).message( :foo ).with( 'some', 'args' ).exactly( 3 ).times

(Not sure #message in the last line would be the best method name, but it illustrates the point.)

John Wilger

Another possibility: separate the injection of the methods from the definition of the stubs.

allow_stubbing_on( my_object )

my_object.stubs( foo: 'bar' )

Or, what about #override instead of (or in addition to) #allow.

(Just brainstorming here.)

David Chelimsky
Owner

How about something like this:

on(obj).expect(message).with(args).and_return(val)
on(obj).stub(message).and_return(val)

The new syntax is only on and expect. Everything else stays the same. Is on too likely to general (i.e. likely to create conflicts)?

David Chelimsky
Owner

@jwilger re my_object.stubs( foo: 'bar' ), we've already got three ways to define return values, so I don't want to add another as part of this.

obj.stub(m => v)
obj.stub(m) { v }
obj.stub(m).and_return(v)

Nice thing about on is all that just changes to:

on(obj).stub(m => v)
on(obj).stub(m) { v }
on(obj).stub(m).and_return(v)

I'm open to a different word, but I'm quickly growing attached to the idea of adding a single wrapper function to allow us not to have to add methods to all objects. This is what flexmock and RR both do (though their syntax is a bit different) and I'm a big fan.

John Wilger

I agree that the idea of having a single wrapper method is the way to go. I'm not a fan of #on, as it doesn't reveal much about what it actually does.

re: my_object.stub(foo: 'bar'); Unless it's about the 's' that I accidentally added to the end of the #stub (I've been working a lot on a project that uses Mocha), I'm not sure what you mean, since that's equivalent to obj.stub(m => v).

David Chelimsky
Owner

I missed the trailing colon. I thought you were saying obj.stub(m, v). Never mind :)

John Wilger

Some other possibilities:

mask(obj).stub(m => v)
mask(obj).expect(m).and_return(v)

screen(obj).stub(m => v)
screen(obj).expect(m).and_return(v)

disguise(obj).stub(m => v)
disguise(obj).expect(m).and_return(v)

relieve(obj).stub(m => v)
relieve(obj).expect(m).and_return(v)

take_over(obj).stub(m => v)
take_over(obj).expect(m).and_return(v)

simulate(obj).stub(m => v)
simulate(obj).expect(m).and_return(v)

mimic(obj).stub(m => v)
mimic(obj).expect(m).and_return(v)

affect(obj).stub(m => v)
affect(obj).expect(m).and_return(v)
John Wilger

Out of those, I think I like #mask the best. When adding a mock or stub method to a real object, you are, in effect, applying a mask. The method name is short, easy to spell, and reveals the intention pretty well.

Myron Marston
Owner

A huge +1 to @dchelimsky's idea of on. I like how it reads a lot. I like that it allows us to preserve the rest of the current fluent interface. I like that it's a very short word (so it doesn't make the code much longer--in fact, on(foo).expect is shorter than foo.should_receive. I like that it works for mocks and stubs.

One side comment: this isn't really fulfilling @iain's original request of using the new rspec-expectations syntax for mocks, but we've seen how difficult it is to come up with a way of using that syntax for mocks and stubs, and I'm more interested in solving the issue of adding stub and should_receive to every object (with the proxy/delegate issues that creates) than in mirroring the syntax of rspec-expectations.

Is on too likely to general (i.e. likely to create conflicts)?

I think it's actually so general that it's unlikely to create conflicts :). That sounds like an oxymoron, but on is a word on the level of the or an or with--it's a grammar word (a preposition in this case) that on its own doesn't really have any meaning, without the surrounding context of other words. I think it only makes sense to be used as part of a fluent interface. I can't imagine anyone having a stand-alone spec helper method called on--it reveals no intention whatsoever. Given that it makes little sense outside of a fluent interface, I find it unlikely that end users will have on defined in their example groups.

That said, if we want to be conservative, we could play it safe and wait to introduce this until 3.0, or introduce it in a future 2.x release, but with the syntax options (similar to rspec-expectations') defaulted to only the current syntax. However, if we do this, I'd like to get it out in a 2.x release with the options defaulting to both syntaxes being available, so the rest of the 2.x can be a transitionary period to the time when we default the old syntax off (potentially 3.0, but that's obviously very, very open to discussion and change).

Iain Hecker
iain commented July 21, 2012

I like on too, but it moves away from a more unified syntax. Now this has never really been the case: it's should_receive and not should receive.

For the same reason, I don't like that the new expect syntax doesn't use blocks, as in: it would be neat if expect { obj }.to eq 5 was also supported. But that is besides the point right now.

We've been coming up with alternative syntaxes for quite a while now, but I fear mocking and stubbing is drifting too far apart from the rest of the RSpec syntax.

We could just deprecate stub in RSpec 2, and change the meaning in RSpec 3. That way we can use expect(object).to receive(method) for mocks and stub(object, :method => value). We got 2 other aliases for stub anyway. Too radical?

Myron Marston
Owner

I like on too, but it moves away from a more unified syntax.

FWIW, I've been thinking about the unified syntax possibilites of expect(obj).to receive a bit today as this discussion goes on, and while I like that it is unified with rspec-expectations, I've been thinking it's probably a good thing to not re-use the same syntax for something entirely different. expect(obj).to matcher fails the example (or not) based on the current state of obj. expect(obj).to receive looks the same but would do something entirely different. It's not confusing for those of us who have been participating in this conversation, but in general, similar-looking constructs should behave similarly and different-looking constructs should behave differently. It could lead to a great deal of confusion on the part of new RSpec users.

For the same reason, I don't like that the new expect syntax doesn't use blocks, as in: it would be neat if expect { obj }.to eq 5 was also supported. But that is besides the point right now.

It could probably theoretically be supported, but I'm not sure what the advantage is. Plus, the new expect syntax does support blocks, for when you are expecting something will happen as the result of running a bit of code (e.g. change, raise_error, throw_symbol, yield_control, etc). I think it would be confusing to support a block for value expectations when a block is already used for block expectations. That's a different discussion, though :).

We could just deprecate stub in RSpec 2, and change the meaning in RSpec 3. That way we can use expect(object).to receive(method) for mocks and stub(object, :method => value). We got 2 other aliases for stub anyway. Too radical?

Even though I'm thinking that unity of syntax w/ rspec-expectations isn't desired here, I do think unity of syntax of mocks and stubs are, particularly when the fluent interface comes into play. It's not clear to me how the stub(object, :method => value) syntax would support that. And changing the meaning of stub would lead to greater pain for those upgrading than introducing a new method (like on) will.

Rodrigo Rosenfeld Rosas

I'm definitely +1 for David's on suggestion

Myron Marston
Owner

Some further thinking of mine regarding on:

  • Rather than on(obj).expect(:message), we could do on(obj).mock(:message). That has some nice symmetry with on(obj).stub(:message) (since we call them "mocks" vs "stubs", not "expects" vs "stubs"). The fact that both are 4 letters means they would line up visually nicely as well. I think it's also closer the vocabulary I use in conversation when discussing tests: in conversations with coworkers, I use mock as a verb all the time (e.g. "I mocked the foo method"). Lastly, given the important of expect for rspec-expectations now, not using it here could be beneficial in terms of using expect for just one thing in rspec (rather than for 2 distinct things).
  • Presumably, we'd also want to make unstub available the same way: on(obj).unstub(:previously_stubbed_message).
  • We need a method available off the wrapper object that corresponds to the current should_not_receive. I don't have any ideas at the moment, though. Neither on(obj).dont_mock nor on(obj).dont_expect read right. Anyone have an idea here?
  • I was thinking that it when you are setting up multiple mocks or stubs on a single object, it might get slightly cumbersome to have to repeatedly wrap it with on. on could take a block for cases like these:
on(obj) do |o|
  o.mock(:foo) { "foo value" }
  o.stub(:bar) { "bar value" }
end
  • For pure mock objects, we could potentially make mock/expect and stub available directly on the object w/o the need to wrap them with on, since they're "owned by RSpec", so to speak, and don't suffer from the delegate/proxy issues. However, it might be good to keep things consistent and, if the on syntax is used, force it to be used on pure mock objects as well. I'm not sure where I fall on this one yet. Regardless, I think we'd still want to support declaring stubs as a message/return-val hash when creating the mock (e.g. double("my double", :foo => 7), and even if we allowed setting mocks or stubs directly on the mock object, it should still also work with on to allow for helper methods that can operate on either a real object or a mock object.

These are all just current thoughts of mine...I'm not wedded to any of this.

David Chelimsky
Owner

On the fence re: "mock". Agree it is concise, and most people will know what it means. The problem is that 10 people will answer "what does mock mean" differently.

Regardless of the word we end up w/, I like on(obj) {|o| ...} and think it should apply to real objects and doubles (what you're referring to as "pure mocks") alike.

re: unstub, what do you think about changing that to restore in the context of on (aliased to unstub for compatibilty): on(obj).restore(:previously_stubbed_method)

re: negative expectations, expect actually works quite nicely: on(obj).expect(:message).never

More thoughts?

Myron Marston
Owner

On the fence re: "mock". Agree it is concise, and most people will know what it means. The problem is that 10 people will answer "what does mock mean" differently.

I'm on the fence, too.

re: unstub, what do you think about changing that to restore in the context of on (aliased to unstub for compatibilty): on(obj).restore(:previously_stubbed_method)

I like how restore reads quite a bit, but I don't know that I prefer it over unstub. I think both read fine. I think I'd slightly prefer unstub to maximize continuity with the current API (since it uses unstub and not restore). Plus, the relationship between stub and unstub is more obvious than stub and restore.

re: negative expectations, expect actually works quite nicely: on(obj).expect(:message).never

That reads fine. I had forgotten about the never qualifier...when I first started using rspec mocks I used it a bit (foo.should_receive(:bar).never) and then discovered should_not_receive, started to use that consistently, and forgot about never.

I think the last major thing to figure out is the config options for the transition period to this syntax. Ideally, it would work like the config option in rspec-expectations for the new syntax, but I'm having a hard time coming up with concise symbols to name the two syntax options.

BTW, is there anyone who wants to take a stab at working on this?

Rodrigo Rosenfeld Rosas

Sinon.js uses "restore" and I find it a better name than "unstub" for readability. Not that I care too much about its name though... :P

http://sinonjs.org/

Avdi Grimm
avdi commented August 27, 2012

Thanks @myronmarston for pointing me to this dicsussion, I've flagged the whole thing for a later read. I'm really happy this is happening, because should_receive has been bothering me more and more lately. FWIW, I like allow a lot; it's consistent with the terminology (not just the code) in "Growing Object Oriented Software Guided by Tests".

Ben Hamill

I am torn between wanting badly to mirror expect(bleh).to receive(:foo).with(:bar) and the ideas of 1) mirroring whatever stub syntax and 2) expect().to receive working differently than expect().to eq (and other matchers).

A part of me really wants all expectations to be expressed the same way. On that front, I like allow for stubbing.

Pat Maddox

HUGE fan of expect and allow language. I think it represents the intent perfectly without getting into muddy mock vs stub vs blah blah waters (even though rspec is obviously clear on meaning of mock / stub / expectation)

Myron Marston

It's funny that we seemed to be gaining some consensus around @dchelimsky's on syntax suggestion above, but no people are chiming in in favor of the older idea expect and allow.

@avdi / @benhamill / @patmaddox -- does the use of allow to stub a method on an object that already has the method bother you? It concerns me a bit, as does the potential confusion from having expect be used for two different things (setting current expectations and setting future message expectations).

Ben Hamill

I'm pretty fine with allow. I agree with the comment above about awkwardness around stubbing a method on a real object being OK and a slight behavior hint.

Avdi Grimm

I'm not bothered by allow being used on a "real" object - I view that as a somewhat degenerate case when all else fails, so I wouldn't want to make naming decisions based on it. That said, I'd support some kind of alias for it to make the case of stubbing on a "real" object read more obviously.

The expect overloading is a little weird, but not a dealbreaker for me.

For the sake of argument, has anyone proposed anything along these lines yet?

the_post = double
allow(Post).to receive(:find).and_return(the_post)
expect(the_post).to receive(:title=).with("New Title")
# ...
Ben Hamill

Also, FWIW, I think the typical RSpec user doesn't know where the line between rspec-expectations and rspec-mocks is. I certainly didn't until I tried to write expect(api).to receive(:request).with(...) the other day and had it blow up. If that's something you want to roll with, then I think there's value in making the syntax look the same between the two. If it's something you want to actively fight (which is to say, educate people about, to be a bit less violent), then a stark difference might help that.

I think a mental model that has receive as a matcher that works on future messages is close enough to let one write good examples still. It may not be technically right in that the difference begins before then, but... I dunno.

If you wanted to get around all of that, I guess you could do something like

expect { subject.whatever }.to call(:message).on(stub)

or something. That mirrors the exception syntax a bit and makes them both kind of... check the block as it executes, rather than making assertions after. I'm not sure. I think I like expect/allow more than that, though.

Avdi Grimm

BTW forgive me if the syntax I just proposed is the exact thing people have been talking about. I have a headache and I hadn't looked at this thread in a while. I was thinking we were talking about some_stub.allow().

David Chelimsky
Owner

A lot of folks have requested support for spies in rspec-mocks. If we figure out how to support expect(obj).to receive(:foo) we can also support expect(obj).to have_received(:foo). Not sure how I feel about that yet, but it would sure make ppl happy, and I personally like the idea of unifying the interfaces (over time) in rspec-mocks and rspec-expectations.

Pat Maddox

@myronmarston sorry I wasn't clear. I like expect(foo).to receive(:blah) (or allow instead of expect, for stubs). I'm not a fan of the on

Avdi Grimm
Myron Marston

One potential gotcha with the expect/allow syntax we've been talking about:

allow(my_test_double).to receive(:bar) do |arg|
  do_something_with(arg)
end

I use the block implementation feature of rspec-mocks a lot, and I just realized that there's a subtle problem here: do/end binds to #to, not to #receive. You have to use curly braces to make it bind properly. This is pretty subtle and will be a potential gotcha for folks using a block implementation since do/end is "normal" syntax most rubyists use for blocks (at least in my experience).

Ben Hamill

Woah. That's is... unexpected. I guess you could also give to some parens, but... bleh on that. Hmmmmmm.

Myron Marston

FWIW, the on syntax we've discussed above doesn't suffer from this gotcha. This would work just fine:

on(my_test_double).stub(:bar) do |arg|
  do_something_with(arg)
end
Avdi Grimm
David Chelimsky
Owner
Avdi Grimm
Myron Marston

I'm curious how common it is to use custom stub implementations.

I use it all the time. In my opinion, it's the single most powerful aspect of rspec-mocks. With just a block implementation you can trivially implement all the other aspects of the fluent interface, plus some more stuff the fluent interface doesn't support (e.g. spies).

That said, I'm clearly a power rspec user given that I'm a committer, so I may not be the norm.

Here's another idea:

expect(foo).to_receive(:bar) do |arg|
end

allow(foo).to_receive(:bar) do |arg|
end

This would solve the problem as the block would clearly bind to to_receive. I think it would also potentially be easier to implement, as expect(foo).to receive forces us to implement a receive matcher that doesn't really act like a matcher.

That said, I know a large part of the appeal of expect(foo).to receive is the consistency of the syntax with rspec-expectations, and this would lose that.

It's amazing how hard it is to come up with the right syntax for this :(.

Avdi Grimm
Avdi Grimm
Pat Maddox

I'm wondering if we can do some magic here...yeah do do...end version binds the block to the to call. Since to doesn't use a block, perhaps it can hold onto it and inject it into the first arg that it receives? Definitely needs some thought because that might paint us into a corner for the future. But I'm not sure. Just a thought.

Rolf Bjaanes

Has this discussion ended?
What was the outcome?.

Toby Ovod-Everett

OK - I'm an idiot - on my first read through of this thread I totally missed Avdi's post in the middle of this thread where he raised the exact same point I just raised below. I'm leaving the rest of my post untouched because I think it adds support to Avdi's comment (and because I find it humorous that I came up with the exact same syntax). Avdi's right, though - overriding Object#send might be unhealthy - it might be safer to change that to send_method or some such.

I'm going to ask a totally random question as a Ruby/Rails/rspec newbie.

What if we made matchers for receiving a method call but instead to call them send?

The syntax that I keep wanting to write (and maybe I can, I just haven't found it anywhere) is something like:

expect { some_block }.to send(my_method).to(receiver).with(args).and_return(response from receiver)

Basically, while I understand intellectually why mocking and stubbing has to be done prior to triggering the code under test, it still feels wrong. That's what lambdas are for, and that's why expect { some_block }.to raise_error is so beautiful - it reads properly. Why can't we use expect in the same manner with mocking?

In a side note, I've been mucking around a lot with DRYing out RSpec/Shoulda test by sticking lambdas all over the place. I have no idea if this is good practice - it feels right to me, but what do I know. For instance, I've started using the following idiom a lot:

let(:action) { block_that_triggers_the_thing_you_want_to_test }
it { expect { action }.to change . . . }
# here go any more tests that use expect with the action in a
# lambda to delay execution until after the tests are set up
context do
  before(:each) { action }
  it { should test_for_action }
  # here go more tests that are pure post-action
end

Basically, I tried to figure out how I could DRY up all the auto-generated rspec-rails tests in the controllers and I thought it was just ugly as all get out that the call to post was showing up in test after test and that there were lots of test with more than one assertion in them. I wanted the thing I was testing to go in one place and the tests to show up in single line it blocks. I'm not entirely happy with the use of action, but I haven't been able to thing of a better word. I don't think subject is appropriate - to my mind subject is the object you're testing, but action is what you're doing to test it. In some cases, those are the same (i.e when the subject is a class method call - frequently I embed it in a lambda and then use subject.call(args).should . . .), but in other scenarios the subject might be a Rails controller while the action is something like post :create, {params hash}, session.

Ben Hamill

There's a lot here, but the biggest thing that jumps out at me is that it's probably a bad idea to override Object#send, even if it's on some RSpec-internal object that's purpose-built to have expectation methods called on it.

Will Madden

A lot of folks have requested support for spies in rspec-mocks. If we figure out how to support expect(obj).to receive(:foo) we can also support expect(obj).to have_received(:foo). Not sure how I feel about that yet, but it would sure make ppl happy, and I personally like the idea of unifying the interfaces (over time) in rspec-mocks and rspec-expectations.

Spies are a much more elegant solution to this problem. #should_receive is an expectation and should reside in rspec-expectations already - it's always bugged me that #should_receive expectations come before the subject and all other expectations come after it.

Why not add a method #has_received? to the stubbed object, then Rspec's inflections would resolve #have_received without needing to create a new matcher? Admittedly, adding a method to the object is a bit gross. Alternatively, you could do expect(spy_on(object, :method)).to have_been_called or something similar.

Myron Marston

@wmadden -- we can have expect(object).to have_received(:some_message) without adding has_received? to every object...we just need to implement a have_received matcher and figure out where to store the state.

Myron Marston

BTW, for those wondering if this conversation has ended (such as @rolfb): no it hasn't. We just haven't been able to come to any kind of consensus. I still want to make a change in this direction at some point (likely as part of RSpec 3) but we haven't yet reached a consensus, and I don't have any more ideas at this point.

Will Madden

@myronmarston doesn't adding a matcher for mocks couple rspec-mocks to rspec-expectations (or vice versa)?

Myron Marston

@wmadden Hmmm...it might have to be a feature that's only available if users are using both rspec-expectations and rspec-mocks. I'd really like to get away from RSpec adding lots of methods to every object.

Matt Bridges

Thanks @myronmarston for pointing me to this thread.

Wow, what a lot to swallow! I'm probably chiming in really late, but I am definitely in favor of the expect(object).to receive(:some_message) for mock expectations and on(object).stub(:msg) for stubbing. allow() is also a very nice alternative, but overall, I like the idea of de-polluting the Object class over simple method chaining.

I'll have to come back and read the whole thing at another point, but that is my quick 2 cents...

Seth Vargo

I'm really in favor of this syntax:

expect(object).to receive(:message)
expect(object).not_to receive(:message)
  1. It's the least friction and delta against the existing should-method
  2. It reads nicely and is very "rubyesque" - some of the examples with blocks feel dirty to me
  3. The introduction of an additional keyword seems unnecessary given expect - it's silly to introduce yet another keyword that a user must remember, especially when their meanings are so similar

Just my 2 cents.

Daniel Harrington

@sethvargo i'd love to write that!

Erik Michaels-Ober

6 months ago @myronmarston commented:

FWIW, I've been thinking about the unified syntax possibilites of expect(obj).to receive a bit today as this discussion goes on, and while I like that it is unified with rspec-expectations, I've been thinking it's probably a good thing to not re-use the same syntax for something entirely different. expect(obj).to matcher fails the example (or not) based on the current state of obj. expect(obj).to receive looks the same but would do something entirely different. It's not confusing for those of us who have been participating in this conversation, but in general, similar-looking constructs should behave similarly and different-looking constructs should behave differently. It could lead to a great deal of confusion on the part of new RSpec users.

I disagree that unifying the expectation syntax would be confusing to new RSpec users.

The reason this syntax makes sense in both cases is because, in both cases, the syntax defines an expectation. What that expectation is (i.e. an expectation about what the object is or an expectation about what the object will receive) is defined after the to (either by a matcher or by receive).

I think it should obvious to everyone–new and experienced RSpec users alike–that:

expect(obj).to eq :foo

will pass iff the object equals :foo and

expect(obj).to receive :foo

will pass iff the object receives the message :foo.

This is not confusing. If anything, I’d argue that new and different syntax is more confusing, if only because of the conceptual overhead of additional syntax.

Erik Michaels-Ober

Furthermore, there are various real-world examples of RSpec users who expected (forgive the pun) this syntax to exists. Namely:

I also expected this syntax to exist and see very little harm in adding it. It seems much better than continuing to monkey patch should_receive and should_not_receive onto every object.

Rolf Bjaanes

I'm in favor of the syntax @sethvargo mentioned, except that I would prefer:

expect(object).to not_receive(:foo)
Will Madden
Seth Vargo

@rolfb @wmadden it's not_to, but yes - it's already part of expectations

Myron Marston
Owner

It makes me happy to see folks commenting on this issue and keeping it alive. I very much want to make a change here; I just haven't had the time to act on it yet, plus I like giving the chance for community discussion so we can arrive at the best syntax.

@rolfb @wmadden it's not_to, but yes - it's already part of expectations

Actually, both to_not and not_to work :). One is aliased for the other. Originally it was to and to_not (corresponding to should and should_not) but then someone pointed out that to_not leads to a split infinitive which is a frowned-upon grammatical construct in some circles...so we added not_to as an alias for to_not and you can use whichever reads better in your situation :).

I disagree that unifying the expectation syntax would be confusing to new RSpec users...

You argue well, @sferik. I withdraw that concern of mine :).

However, there are still a few things to resolve, I think:

  • Unstubbing: currently you can unstub a previously stubbed with obj.unstub(:foo). Given that our stubbing syntax will be allow(obj).to receive(:foo)...what would the unstub syntax be?
  • As with the syntax change in rspec-expectations, I want to add a config option to allow users to choose which syntaxes are enable. However, I can't think of good names for them here; in rspec-expectations it's expect vs should which are very short and clear. Here's it's like should_receive_and_stub vs expect_and_allow. Anyone have a suggestion here?
  • As I commented on above, the block implementation feature (e.g. obj.stub(:foo) { :some_value }) has a gotcha in that do/end will bind to to, not to receive. You have to use curlies to bind it properly to receive. @patmaddox suggested adding some logic to "forward" the block from to to the matcher, and on the surface, I like this idea if we can easily do it consistently with minimal complexity. I didn't realize this, but it looks like rspec-expectations already forwards the block on when it calls matches? on the matcher...so we may be able to forward that block on as well. I'll have to think about it carefully, though. If there's not a clean way to make this work, we should at least be able to make it raise a "use curlies instead of do/end" error so the block isn't silently ignored.

Anyhow, I'll try to make this my top priority for when I next have time to work on a new RSpec feature. I expect it'll be at least a few weeks before I get around to it, though; I'm currently in a coursera class (Algorithms 2) that runs another 2-3 weeks and it's limiting my time to work on open source.

Ben Hamill

Unstubbing: currently you can unstub a previously stubbed with obj.unstub(:foo). Given that our stubbing syntax will be allow(obj).to receive(:foo)...what would the unstub syntax be?

Well, just grammatically, the opposite would be disallow, though that doesn't quite have the right meaning and doesn't take the right preposition (disallow(obj).from receiving(:foo) terrible). I assume keeping to(receive(:foo)) in there is desirable? Or do we care about that at all?

As with the syntax change in rspec-expectations, I want to add a config option to allow users to choose which syntaxes are enable. However, I can't think of good names for them here; in rspec-expectations it's expect vs should which are very short and clear. Here's it's like should_receive_and_stub vs expect_and_allow. Anyone have a suggestion here?

With adequate documentation, I think the same settings (expect vs allow) would make sense. Or, if it really needs to be different, the long names you proposed don't offend me.

Myron Marston
Owner

Well, just grammatically, the opposite would be disallow, though that doesn't quite have the right meaning and doesn't take the right preposition (disallow(obj).from receiving(:foo) terrible).

Yeah, I thought of that, but disallow(obj).from receiving(:foo) doesn't really mean the same thing as unstub

I assume keeping to(receive(:foo)) in there is desirable? Or do we care about that at all?

I'd like for the syntax to stick as close as possible, but I'm open to any ideas, really.

With adequate documentation, I think the same settings (expect vs allow) would make sense. Or, if it really needs to be different, the long names you proposed don't offend me.

The problem is, it's not expect vs allow--it's the old syntax (should_receive and stub) vs. the new syntax (expect and allow).

Seth Vargo

@myronmarston WRT to stubbing/unstubbing, I propose something like this:

# Expectations
expect(object).to receive(:foo) # with(param)

# Stubbing
stub(object).my_method # with(param).and_return(:foo)
stub(MyClass).my_class_method 
# or even
stub(object).my_method(arg1, arg2) # and_return(:foo)

# Unstubbing
unstub(object).my_method

Is there a reason we don't just leverage stub and unstub as methods directly? Sorry if I missed something in the thread.

  1. I think this syntax "reads" more how I think

    Stub MyClass.my_method and return ":foo"

  2. It's fairly easy to implement, since you control the returning object from stub/unstub.

  3. It allows you to leverage unstub again, which IMO is the only viable nomenclature for such an operation.


WRT to the config option, why not leverage the existing :expect and :should? I don't see the advantage in mixing and matching and/or introducing yet another configuration option. It'll quickly result in a complex matrix with all the rspec plugins out there:

 rspec-expectations-expect  |  rspec-mocks-expect  |  rspec-dns-expect
-----------------------------------------------------------------------
 rspec-expectations-expect  |  rspec-mocks-expect  |  rspec-dns-should
-----------------------------------------------------------------------
 rspec-expectations-expect  |  rspec-mocks-should  |  rspec-dns-expect
-----------------------------------------------------------------------
 rspec-expectations-expect  |  rspec-mocks-should  |  rspec-dns-should
-----------------------------------------------------------------------
 rspec-expectations-should  |  rspec-mocks-expect  |  rspec-dns-expect
-----------------------------------------------------------------------
 rspec-expectations-should  |  rspec-mocks-expect  |  rspec-dns-should
-----------------------------------------------------------------------
 rspec-expectations-should  |  rspec-mocks-should  |  rspec-dns-expect
-----------------------------------------------------------------------
 rspec-expectations-should  |  rspec-mocks-should  |  rspec-dns-should

That means someone wanting to contribute to an open source project has a large number of permutations of syntax configurations he/she must juggle. Project X uses expect, but should for mocks, Project Y is the opposite, and Project Z uses all should.

I think rspec plugins supporting both syntaxes should support the expect and should rspec config option, and nothing more.

David Chelimsky
Owner

@sethvargo the problem is that there is already a stub method that generates a test double. Using it as you suggest would require a very painful change for many users.

Seth Vargo

@dchelimsky Why can't it support both? If I recall correctly, stub (when used as a double), takes a String parameter. It should be easy enough to overload stub to behave differently when given a String vs an Object or Class.

Granted, there's an edge case I completely ignored of "Stubbing a String", but I think it's possible to do both.

I also think stub (as a double) should be deprecated in favor of the more explicit double.

Seth Vargo

For example (and this is totally off the top of my head):

class Stubber
  def self.stub(obj)
    if obj.is_a?(String)
      # Logic to double
      Doubler.new(obj)
    else
      # Logic to stub
      new(obj)
    end
  end
end
Seth Vargo

Like I said, the only edge case (I can think of) is:

stub(String).i_monkey_patched_string_and_really_shouldnt_have_method and_return(false)

I see no value in something like this:

stub('foo').my_upcase and_return('FOO')

So you could even just leverage instance_of? instead of is_a?:

class Stubber
  def self.stub(obj)
    if obj.instance_of?(String)
      # Logic to double
      Doubler.new(obj)
    else
      # Logic to stub
      new(obj)
    end
  end
end

I'm fairly certain even that covers the edge case.

David Chelimsky
Owner

@sethvargo it never required a String - just something that responded to to_s, so there is a good chance there is code out there that would break e.g. thing = stub(Thing, :do_something => 42), so we'd be risking breaking suites in surprising ways.

I would personally love to see mock and stub go away as aliases of double (they're not actually aliases, but all three return instances of the same class), but there is a lot of risk associated w/ doing that.

Keep in mind that RSpec is used by other libraries. This means that when we make a change that either deprecates or changes behavior in backward-incompatible ways, we run the risk that an end user can't do anything about it even if we give them helpful messages.

Seth Vargo

@dchelimsky I understand that, but I also think that libraries should be locking to a specific version. And rspec follows semver. So breaking API compatibility will require a major release, but it's not out of the question.

I think it's more senseless to introduce additional verbage in order to support backward-compatibility.

There are a lot of gems that break backward compatibility, and while I don't condone it, there are certain cases where you just need to "cut the cord" (for lack of a better phrase).

Keep in mind that RSpec is used by other libraries. This means that when we make a change that either deprecates or changes behavior in backward-incompatible ways, we run the risk that an end user can't do anything about it even if we give them helpful messages.

Like I said before... we aren't breaking anything. Their library is locked to a specific patch level (or minor) of rspec... Releasing a new version as a major wouldn't affect them at all.

Seth Vargo

ORRRR (idea moment):

The expect syntax is "new". We are already discussing it as a configuration option...

You could have stub deprecated under the expect syntax, but still supported under the old should syntax!

Given that the should syntax will most likely be deprecated in the future, the old stub and mock for doubles can follow suit thereafter.

That doesn't break backward compatibility and provides the best of both worlds.

David Chelimsky
Owner

I also think that libraries should be locking to a specific version

Me too, but they don't. Plenty of gem authors use ">= X.Y" or confuse "~> X.Y" or "~> X.Y.X". Remember when Rails was depending on rake, ">= 0.8.7" and the 0.9 release of rake resulted in a wealth of pain?

There are a lot of gems that break backward compatibility, and while I don't condone it, there are certain cases where you just need to "cut the cord" (for lack of a better phrase).

Again, the implications are different when an application depends directly on the gem that's breaking compatibility and when an app depends on a gem that depends on a gem that breaks compatibility.

You could have stub deprecated under the expect syntax, but still supported under the old should syntax!

I like this idea, in that moves us in a good direction and reduces (not eliminates) the risks I just described. In the end, @myronmarston is the one who is going to have to deal with unhappy users if any of these risks pan out, so let's see what his thoughts are on all of this.

Seth Vargo

Me too, but they don't. Plenty of gem authors use ">= X.Y" or confuse "~> X.Y" or "~> X.Y.X". Remember when Rails was depending on rake, ">= 0.8.7" and the 0.9 release of rake resulted in a wealth of pain?

Authors will learn through their mistakes though - I've changed the way I lock versions in a gemspec because I've been bitten in the past. With an author be unhappy - yes, but it's a relatively painless change (locking to the old version) that could easily be described in the CHANGELOG, README, etc.

I think the "advantage" (note quotes) rspec has over rake is the field of use.

If we totally break rspec, there will be a lot of unhappy people/developers. But businesses (using rspec) will continue to make money.

The rake issue (or any production-level dependency) has a noticeable, numeric business impact.

I'm not saying that's an excuse, but it is different. Breaking rspec will cause tests to fail, but the application will still start.

David Chelimsky
Owner

Authors will learn through their mistakes though - I've changed the way I lock versions in a gemspec because I've been bitten in the past.

That works when the author is still maintaining the gem and paying attention :)

I'm not saying that's an excuse, but it is different. Breaking rspec will cause tests to fail, but the application will still start.

That's not a tradeoff I'd accept for myself, much less impose on anybody else.

To be clear, I like the outcome you envision and support such a change, but only if it can be done in a way that doesn't force anybody to monkey patch mid-stream gems and also minimizes upgrade pain (i.e. offers config options that devs can use to say "I'll change all those 'stub' calls to 'double' later, but right now I just want to get this upgrade over and have everything work").

Avdi Grimm
Seth Vargo

@avdi But you shouldn' be using stub to create doubles... That was a discussion on backward compatibility

David Chelimsky
Owner

To clarify one thing: I don't support supporting different behavior based on the type of the argument. I do support (conditionally, as described earlier this thread) moving toward using stub to stub a method on an object instead of using it to generate a test double.

Seth Vargo

I also agree with @dchelimsky at this point

Rodrigo Rosenfeld Rosas

I'd be happy to write specs like:

expect(obj).to receive(:call).with(...).and_return(...)

And if I ever have to unstub it (I never needed it since it is automatically unstubbed after each example, have someone here ever unstubed something?):

restore obj # undo all stubs
restore obj, :call # restore only the :call stub above

It is a bit hard to understand exactly what would be the requirements for such a restore (or unstub) method since I never used it in any project I've worked with. I'll try to look for unstub on GitHub soon to understand what it would be used for...

Seth Vargo

@rosenfeld I feel the same... I feel like if you need to unstub, you should just move into a different context, but I think there's probably a use case for it.

Rodrigo Rosenfeld Rosas

I couldn't find any here after looking at several pages: https://github.com/search?q=unstub&p=1&ref=searchbar&type=Code&l=Ruby

David Chelimsky
Owner

unstub was added in 384ca8 by @Bantik and @niessner (iirc at an RSpec hackfest hosted at Obtiva) - can either of you guys remind us of the use case for it?

Michael Niessner
Seth Vargo

Perhaps it can go? Especially since, as @rosenfeld pointed out, no one (on Github) is using it.

Toby Ovod-Everett

I'm using it. It's useful for a project where you want to globally stub something in spec_helper.rb so that you don't have to remember to stub it all over the place, but then in the actual tests for that code it can be useful to unstub it to verify that the method is working properly. In fact, I'm having to use the alias unstub! because Mocha (which gets included by shoulda-matchers by way of bourne) overwrites unstub, but leaves the alias alone. See #122 for a thread on this that indicates I'm not the only one.

Seth Vargo

@tovodeverett that seems like a very bad idea. You should be using shared examples instead of globally stubbing an object.

Toby Ovod-Everett

In this case, wouldn't it be shared context, not shared examples?

Actually, I suspect I'd be better off modifying the class in question to add a pair of class methods that effectively stub or unstub the method/behavior in question. Given that I'm stubbing a method on a class I control, it would be very easy to do (in fact, I think I'm going to go do that).

Seth Vargo

Sorry, you're right. That's what I meant. (Cracks open another caffeinated drink).

Myron Marston
Owner

Thanks everyone for keeping this discussion going. I'm glad we have a passionate community of users who can help us figure this stuff out :).

To respond to some specific points:

@myronmarston WRT to stubbing/unstubbing, I propose something like this:
Stubbing
stub(object).my_method # with(param).and_return(:foo)
stub(MyClass).my_class_method

That's a very, very different syntax then anything we've proposed here. It calls a method corresponding to the method you want to stub (as RR does) rather than calling a standardized method, passing the method you want to stub as a symbol argument. It also bifurcates the rspec-mocks syntax into one syntax style for a mocked method and another syntax style for a stubbed method (on top of the older syntax we already have). I don't want to introduce two new syntaxes here.

WRT to the config option, why not leverage the existing :expect and :should?

Because it's an rspec-expectations config option, not a general rspec option. On top of that, many projects have already switched to the new rspec-expectations syntax (bundler and many of @sferik's projects come to mind) but they obviously haven't started using a new mock syntax since we haven't created it yet...which means that they need to be able to upgrade and change their config to the new syntax in their own time. We can't use the existing option to mean a change to the syntax here as well, or we'll cause lots of breakage during upgrades.

Anyhow, I still think that allow is the best proposal for a stubbing syntax. It mirrors expect exactly:

expect(obj).to receive(:foo).with(arg)
allow(obj).to receive(:foo).with(arg)

I like the idea of making trimming down the number of ways to create a test double to just double (i.e. deprecating stub and mock for this purpose) but I'm not on board with changing the meaning/semantics of stub to something entirely new as part of this syntax change. Having a method that has entirely different semantics depending on a config option is a recipe for disaster. It creates confusion for folks contributing to a project that uses rspec (e.g. "what does stub mean here? I have to go look at all the files where rspec may be configured to figure it out), and imagine the confusion for newcomers to rspec. Or just think about the complexity of the API docs for stub: "does X when config option Y is set to Z; or when config option Y is set to W, does this entirely different thing". I can just imagine the tickets we're going to get if we make that change...

So, I'm planning to go with expect and allow. Deprecating stub and mock as alternates for double can be a separate change. If someone really wants to use stub over allow then they're welcome to alias it since ruby makes that so easy :).

Finally, given that unstub is used so rarely, and what sounds like the main use case (being able to almost globally stub something) has a better solution now that we have shared contexts, my inclination is to not worry about bringing it forward to the new syntax.

John Wilger
Jeff Felchner

I got to this discussion way too late, but I'm really glad the decision everyone came to. The consistency of the expect syntax and the new allow syntax reads great.

Because it was brought up earlier in the discussion and because looking for spies is what led me here, I just wanted to make sure we come back to that point, and finalize a syntax for, test spies as well. For the purposes of keeping the discussion moving forward, I'm going to summarize here. Both of these syntaxes I like very much.

Current

Our current syntax (without spies) would produce something like this:

# http://www.imdb.com/title/tt0102926/
context 'when Buffalo Bill gives his captive the hose' do
  it 'puts the lotion in the basket' do
    expect(basket).to receive(:<<).with(lotion).once

    BuffaloBill.spray_water_on_captive
  end
end

Option 1

Earlier in this PR it was proposed to do something like this:

context 'when Buffalo Bill gives his captive the hose' do
  it 'puts the lotion in the basket' do
    expect { BuffaloBill.spray_water_on_captive }.to send(:<<).to(basket).with(lotion).once
  end
end

And it appeared that the only stumbling block was around whether to override send in the context of the matcher. Readability-wise, I think this is great, although it is quite a bit to chew off one one line.

Option 2

Another option that was proposed was have_received which also has its advantages.

context 'when Buffalo Bill gives his captive the hose' do
  it 'puts the lotion in the basket' do
    allow(basket).to receive(:<<)

    BuffaloBill.spray_water_on_captive

    expect(basket).to have_received(:<<).with(lotion).once
  end
end

Also a nice option.

My Opinion

Honestly I kind of like the second option slightly more because building up the stub may need to be done in a separate context which would make option 1 basically unusable.

Thoughts everyone?

Myron Marston
Owner

@jfelchner -- I'm glad to discuss spies as a possible feature for inclusion in a future rspec-mocks version, but I think it's a separate issue from this one (though it may build on top of the syntax discussed here). Can you open a separate ticket so we can discuss it there? I think your comment is likely to get "lost" on this issue, given how far down it is in the discussion. Thanks!

Myron Marston myronmarston referenced this issue in rspec/rspec-expectations February 23, 2013
Closed

New should_receive syntax #214

Jon Rowe
Collaborator

What was the outcome of this? Is it still awaiting future development?

Myron Marston
Owner

What was the outcome of this? Is it still awaiting future development?

I'm still planning on working on it; other, more immediate issues have been taking my time recently. But this is next on my list of "new features" to work on.

Jon Rowe
Collaborator

:+1:

Myron Marston myronmarston referenced this issue from a commit March 17, 2013
Myron Marston Move `stub_chain` functionality out of `RSpec::Mocks::Methods`
...while maintaining the same API.

This allows us to remove the `format_chain` method from every object and
begins to prepare us for the future addition of a new expect-based syntax
to rspec-mocks (#153).
1461794
Myron Marston myronmarston referenced this issue from a commit March 24, 2013
Commit has since been removed from the repository and is no longer available.
Myron Marston myronmarston referenced this issue from a commit March 18, 2013
Myron Marston Externalize methods and state that was previously held on all objects.
- Remove `@mock_proxy` ivar
- Remove `rspec_verify`, `rspec_reset`, `__mock_proxy` and `__remove_mock_proxy` methods.

This is a refactoring that I'm doing to pave the way for the new expect
syntax (#153). As that syntax "wraps" objects in order to mock or stub
them, we need to externalize these methods and state. The methods directly
on the object (e.g. `stub` and `should_receive`) will simply delegate to
this external logic.

This refactoring had some nice side benefits:

- No need for the hacky YAML fix. It was only needed because we were storing
  `@mock_proxy` on the mocked object.
- AnyInstance no longer needs to much with `dup`; again, this was only needed
  because it dup'd the `@mock_proxy` ivar.
- The Marshal extension gets significantly simpler (again, due to not
  storing `@mock_proxy` on the object anymore).

Rather than storing the mock proxies on the objects, we are now storing
them in a hash, keyed by object_id.  This is pretty simple and consistent,
but could be problematic for objects that muck with `object_id`. That method
seems a bit sacred, though, so I'm not too worried about it.
210c9f0
Myron Marston
Owner

I've started working on this feature in the new_syntax branch, for anyone who cares to try it out or follow along. A few things to note:

  • We never discussed any_instance on this ticket before. For now, I'm going with expect_any_instance_of(MyClass).to receive and allow_any_instance_of(MyClass).to receive. @alindeman also put up a gist with some syntax ideas (which try to address the occasionally confusing use of any). Feedback welcome.
  • There are a couple features available on the current syntax that I can't think of a good syntax for and will probably not make the initial cut (unless someone can come up with good syntaxes). These are features I personally don't use much so I'm OK with it but I'm curious what others think:
    • stub_chain -- This is usually a code smell, anyway.
    • my_double.stub(:msg_1 => "value", :msg_2 => "value") as a shorthand for my_double.stub(:msg_1).and_return("value"); my_double.stub(:msg_2).and_return("value"). IMO the main time the hash syntax is useful is when creating a test double, and you can still pass a hash to double and it will be treated in this fashion.
  • I'm planning to include this in RSpec 2.14. (Along with the test spies feature @jferris added recently).

Thanks to everyone for your patience on this.

Sam Phippen
Collaborator

@myronmarston can this be closed now #266 is merged?

Myron Marston myronmarston closed this May 10, 2013
Leif Gensert

Just a general question:

does a pendant for unstub exist in the new expect syntax?

And if not, wouldn't it be good to have an according notice in the deprecation message.

All I currently (3.0 beta1) get is:

sing `unstub` from rspec-mocks' old `:should` syntax without explicitly enabling the syntax is deprecated. Use the new `:expect` syntax or explicitly enable `:should` instead. Called from some_spec.rb:23:in `block (2 levels) in <top (required)>'.
Jon Rowe
Collaborator

I don't believe we have a replacement for unstub we should probably add a note that it's only available using :should.

As an aside I've always seen it as a test smell, so I don't think it's worth bring across.

Zsolt Fabok ZsoltFabok referenced this issue from a commit in ZsoltFabok/leankitkanban January 09, 2014
Zsolt Fabok adding rake and fixing deprecation issues:
 - rspec using double instead of mock: rspec/rspec-mocks#153 (comment)
 - rspec not_to raise exception with class:  rspec/rspec-expectations#231
9df3482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.