David Chelimsky

I agree that the original pattern feels clunky, and that we should do something to fix it. And the :before => do_post syntax isn’t clear whether the do_post runs before the spec, or if the spec should run before the do_post.

At a basic level, what we want is to add a before clause (that runs before the standard before clause) that is specific to an individual spec. So, I’d prefer it to look something like this:

describe PersonController, "handling failed POST to create" do
  before(:each)
    post :create, invalid_arguments
  end

  it "should redisplay the create form" do
    response.should render_template("people/new")
  end

  it "should try to create a Person" do
    before_the_before_clause do
      Person.should_receive(:create).with(invalid_arguments).and_return(false)
    end
  end
end

I’m not sure how hard it would be to implement that though. I would guess that it would require a serious architectural change to RSpec.

I think the case mainly arises due to the way we do mocking. My suggestion would be to do the stubbing in the before clause, and separate the mock assertions, moving them into the specs:

describe PersonController, "handling failed POST to create" do
  before(:each)
    Person.upon_receiving(:create).with(invalid_arguments).should_return(false)
    post :create, invalid_arguments
  end

  it "should redisplay the create form" do
    response.should render_template("people/new")
  end

  it "should try to create a Person" do
    Person.should_have_received(:create).with(invalid_arguments)
  end
end

I understand that we’d probably have to re-write a lot of the mocking and stubbing code, but I think that this expresses intent a lot better, and probably solves the root of the problem you’re trying to solve. If something like this exists, I’d be interested in learning about it. If not, I’d be willing to help make it come about.

I actually like the verbosity of this style; this is definitely something I would adopt. I’d actually like to see this implemented into RSpec. I’ll ask around and see what others have to say. So far, I like it!

I think I might have been the one to originally suggest the tags version. I like your version better. :-) I’m going to start trying it out.

I like the distinction between before/during. Even though they are the “same” thing from a coding standpoint, they represent subtly different ideas from a “why do I care” standpoint. It reminds me of Brian Marick’s recent mock-related posts: http://www.exampler.com/blog/2007/10/02/code-sample/

@Craig: I’m interested in something similar to your second example. should_have_received seems like a nice addition to the mock library.

@craig & @nicholas:

I like the should_have_received idea in conjunction with upon_receipt_of or some such. The idea being that they MUST be a pair – you can’t just say retroactively should_have_received. The reason is that you need to set up how the object will respond before the action occurs. So upon_receipt_of would be like an optionally-verifiable stub, w/ should_have_received being the optional verification.

WDYT?

@david:

That sounds reasonable to me. Would there be any functional difference between “foo.upon_receipt_of(:bar)” and “foo.should_receive(:bar).any_number_of_times”? If not, it seems to me that “any_number_of_times becomes” unnecessary.

how about using upon_receiving instead of upon_receipt_of. I dunno why… it just feels simpler to me.

One of my major usage patterns now is to override a stub (in the before) with a mock expectation in one of the example blocks. Unfortunately, rspec stubs don’t allow as fine-grained expectations, and rspec mocks aren’t overridden very easily. upon_receiving/should_have_received could probably replace that pattern most of the time.

My only nitpick is that the upon_receiving/should_have_received pair doesn’t feel as DRY as I’d like it to… but I can tolerate slightly verbose DAMP tests, and I can’t think of a better suggestion at the moment.

@David:

Yes, the mock check (should_have_received) would require that the stub (upon_receipt_of) had been set up before the call to the method in question. The stub part would set up monitoring/recording of all calls it receives, and the mock check would review those calls.

I’ve not looked into the recording-type mocking libraries (in other languages), but I suppose they might be similar to what I’m looking for.

Thinking about it some more, I think it might not be all that hard to implement. There’s already implicit mock checking after each spec is run, so doing the same thing with explicit checks shouldn’t be too hard.

@nicholas:

There’s a significant difference between should_receive and upon_receiving—should_receive asserts that the method MUST be called, where upon_receiving only says what to do IF the method is called.

The any_number_of_times would be added to the bottom should_have_received part. Basically, you default the number of times that the method should have been called to either 1 or “any”, and add any_number_of_times or x_number_of_times to change the default.

It might feel more DRY to you if you omit the with() clause in the stub/setup. Unless you want to return different values for different calls, there’s no point in adding the with() part. And in the mock check at the bottom, you could also omit it, if you don’t care about what arguments were sent.

I agree that it doesn’t feel as DRY as it could, duplicating the callee and the method name, and perhaps the arguments. But that’s because you’re doing 2 different things, and if you DRY it out by combining the 2 things, you end up in the situation that lead to the bad pattern that David first described.

craig: @should_receive(:foo).any_number_of_times doesn’t actually make any assertions. any means zero or more. Obviously, it reads a bit oddly, because it looks like it is making an assertion, but for all practical purposes it isn’t. My only use case for should_receive(:foo).any_number_of_times is when stub!(:foo) isn’t powerful enough to get the job done.

My question is, does upon_receiving(:foo) need to do anything more (or less) than should_receive(:foo).any_number_of_times currently does? If not, then upon_receiving(:foo) should be a drop-in replacement, and the clunkier any_number_of_times should be deprecated (IMHO).

As for the DRY concerns… I think I’d rather work with a while before making any strong suggestions… but I was thinking about something like the following:

describe PersonController, "handling failed POST to create" do
  before(:each)
    @expectation = Person.upon_receiving(:create).
                          with(invalid_arguments).
                          should_return(false)
    post :create, invalid_arguments
  end

  it "should redisplay the create form" do
    response.should render_template("people/new")
  end

  it "should try to create a Person" do
    @expectation.should_have_received(:create)
  end
end

But does that read well?

@nicholas:

Sorry, I forgot that any_number_of_times includes 0 times. So yes, I suppose it’s what I’m looking for, at least as long as I can check later to see if it was called, how many times, and with what parameters.

I don’t think the assignment to @expectation works well, at least not with the current wording. (I really do like the idea though.) It’s not the expectation that should_have_received the method call, so it doesn’t quite sound right.

I’m also starting to think that should_return is poorly worded. With the word “should”, it sounds like an assertion. Perhaps will_return, shall_return, or returns. Also note that in the setup, with(arguments) is a filter, so that only calls with those arguments passed are stubbed. More likely, you’d want to specify it in the spec, where it would be an assertion that the parameters were passed in the call.

@craig:

I agree with you on the wording. @expectation would need to have a different method call than should_have_received. It doesn’t need the method name to be reiterated either. But I can’t come up with anything better at the moment… Oh well. Also, you are right that should_return doesn’t match up well with upon_receiving. I like will_return the best, there.

I don’t know whether using with(arguments) as a filter in the before or as an assertion in the spec is more likely. I do it both ways, depending on the situation. So I’d like both to be supported.

David,

I’m actually using the same approach you suggested to think about. Helper methods with blocks are helpful for exactly this purpose.

Go for it I should say.