Use the new expectation syntax for mocks

[iain]

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.

[myronmarston]

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?

[rosenfeld]

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]

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".

[benhamill]

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.

[patmaddox]

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)

[myronmarston]

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).

[benhamill]

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]

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")
# ...

[benhamill]

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]

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().

[dchelimsky]

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.

[patmaddox]

@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]

On Tue, Sep 11, 2012 at 1:23 PM, Ben Hamill notifications@github.comwrote:

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

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

I was thinking abut that a bit myself today, although I'd prefer to send(:message).to(stub) (even though that means overriding Object#send. I think that would be more in line with the OO messaging paradigm and consistent with receive().

In any case, I think this would make sense in addition to, rather than instead of, traditional expectations using e.g. expect(stub).to receive(:message). Maybe something to add further down the road.

[myronmarston]

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).

[benhamill]

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

[myronmarston]

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]

Blurg. That sucks, especially since I dislike the on() syntax more the longer I look at it. The normal way to say "on this object" in Ruby is a dot. Whereas allow/expect says a lot more about the actual intent of the example.

[dchelimsky]

You had me at "blurg"

On Sep 13, 2012, at 1:26 PM, Avdi Grimm notifications@github.com wrote:

Blurg. That sucks, especially since I dislike the on() syntax more the longer I look at it. The normal way to say "on this object" in Ruby is a dot. Whereas allow/expect says a lot more about the actual intent of the example. — Reply to this email directly or view it on GitHub.

[avdi]

I'm curious how common it is to use custom stub implementations. I rarely use it, and I've hardly ever seen it in all the codebases I've looked at. IME most rspec users are unaware it's even possible.

Just wondering if it's worth optimizing for minimum surprise if the use case is uncommon.

[myronmarston]

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]

FWIW I work with a lot of novices, and this lack of consistency would confuse the hell out of them. It's hard enough helping them understand that sometimes there's an underscore and sometimes a space in existing syntax. Having both 'to_' and '. to ' would be asking too much of them IMHO.

Which is a shame because otherwise I think it's a nice compromise.

[avdi]

Okay, I probably won't be able to respond much more today. Time for the last leg of my flight...

[patmaddox]

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.

[rolfb]

Has this discussion ended? What was the outcome?.

[tovodeverett]

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.

[benhamill]

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.

[wmadden]

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.

[myronmarston]

@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.

[myronmarston]

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.

[wmadden]

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

[myronmarston]

@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.

[mdespuits]

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...

[sethvargo]

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.

[rubiii]

@sethvargo i'd love to write that!

[sferik]

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.

[sferik]

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

• This comment by @rosenfeld
• This tweet by @sethvargo
• This issue (the one you're looking at) by @iain

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.

[rolfb]

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

expect(object).to not_receive(:foo)

[wmadden]

I think to_not has already been established by rspec-expectations. On 12/01/2013 9:01 PM, "Rolf Bjaanes" notifications@github.com wrote:

I'm in favor of the syntax @sethvargo https://github.com/sethvargomentioned, except that I would prefer:

expect(object).to not_receive(:foo)

— Reply to this email directly or view it on GitHubhttps://github.com/rspec/rspec-mocks/issues/153#issuecomment-12183409.

[sethvargo]

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

[myronmarston]

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.

[benhamill]

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.

[myronmarston]

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).

[sethvargo]

@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.

[dchelimsky]

@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.

[sethvargo]

@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.

[sethvargo]

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

[sethvargo]

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.

[dchelimsky]

@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.

[sethvargo]

@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.

[sethvargo]

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.