Support stubs on any instance of a class

[dchelimsky]

Support stubs on any instance of a class

[kaiwren]

So this patch doesn't cover and_yield and and_raise - but if someone can validate the approach I can then add support for them trivially. I've also forked it and patched it at kaiwren/rspec-mocks@b786b96

[dchelimsky]

@SidU - I merged your patch into a branch: http://github.com/rspec/rspec-mocks/tree/any-instance. I also refactored a bit - explained in the commit message: http://github.com/rspec/rspec-mocks/commit/c2e8c59561261c7b22253d97424c80eefba1bd40

In addition to @kaiwren's comments re: and_yield and and_raise, we need to make sure the class is restored to it's original state after each example.

@kaiwren, I'd like to give @SidU the opportunity to address these issues first, so let's wait to hear back from Sidu.

[kaiwren]

My bad - I should have thought to mention that I am Sidu :). I'll take a look at your changes and clean things up appropriately.

[kaiwren]

'#any_instance with a block returns the same computed value for calls on different instances' currently fails. I just wanted to confirm this is intentional and is represents something that still needs to be implemented?

[dchelimsky]

Oversight on my part - Object.new != Object.new :)

Just change the content of the block to something like 1 + 2. The expectation is that the block will be eval'd each time, so a different object would likely result, but that it should compute the same result each time. Just like stubs directly on instances do. Make sense?

[kaiwren]

Perfectly. I'll get this done and add and_raise and and_yield tonight, my time (I'm at +5:30). Thanks for responding so quickly :)

[kaiwren]

Please take a look at http://github.com/kaiwren/rspec-mocks/commit/5788fbe3fcfaa930f87b42f08ec70310dbfc77da and let me know if this implemetation (and the specs) suffice to ensure that the class is reset after every example.

I've also ensured that the stub chain on #any_instance must now be called in the right order i.e
klass.any_instance.with(1).stub(:foo)

now raises a NoMethodError as one would expect

[kaiwren]

I'm not sure how to test that a class is added to the current space on a call to #any_instance. Please let me know how I can write a spec for this.

[dchelimsky]

@kaiwren - just inspect the contents of @Mocks in RSpec::Mocks.space or make mocks public and get at it that way.

Also - new needs to be un-decorated.

[kaiwren]

Just like me to miss the obvious (undecorating new). I'll take care of it.

[oriolgual]

Any news on when this will be merged to master?

Thanks for your hard work!

[dchelimsky]

There are still loose ends in the patch. Once resolved it won't be long before it's merged.

[kaiwren]

I'm stumped - I simply can't seem to undecorate new without a bunch of stuff going wrong. Could you take a look please? It looks like I'm running afoul of method_double when removing the decorated new. I'm also seeing different behaviour on 1.9.2 from 1.8.7.

http://github.com/c42engineering/rspec-mocks/commit/34ae8408089c485ce005675e385cc3e05f6c8c68

[alindeman]

Any news on this? Having any_instance type stubbing/mocking would make my life much easier in certain cases.

I'd be happy to try to pick up the development here if it's stalled.

Thoughts?

[kaiwren]

David, when we met at RubyConf in New Orleans, you'd mentioned that you were thinking of re-working (or possibly re-writing) rspec mocks. I finally have enough time that I can take a decent stab at it, so please let me know if you're still thinking along those lines and have the time to guide me.

[dchelimsky]

@kaiwren - the changes to the mock framework won't happen anytime soon due to other priorities and limited resources. If you have time to work on any_instance, I say go for it. We have a bigger team now, and any of us can answer questions about this.

[alindeman]

Awesome: I just want to see it done. Let me know if I can help :)

[kaiwren]

Fair enough. I'll see if I can think of a work around for the issues with instances of Strings, Arrays, Hashes and Classes that are created using their respective interpreter hacks instead of using new (i.e. "", [], {} and class respectively). That's what had me stumped the last time around.

[alindeman]

Maybe investigate how mocha achieves it?

[kaiwren]

@alindeman take a look at this spec that I just checked in - this is what I came up with in my last attempt plus a few extra to cover [] etc. that we need to get passing.

[kaiwren]

I think I've got it sorted on c42engineering/rspec-mocks@4bf0d5b - my build is now green on 1.9.2-p136. (I'd left in a require 'ruby-debug' by mistake that I've removed in a subsequent commit).
@dchelimsky - let me know if this looks ok and I'll then test on 1.8.7, JRuby and rbx.

[dchelimsky]

Looks good to me. Please proceed.

[kaiwren]

I've added support for and_raise got a passing build on 1.9.2-p136 and 1.8.7-p330.

1.8.6-p399 fails on line 103 of any_instance.rb because of the changes to blocks passed to block syntax. Any advice on working around this in 1.8.6?

jruby 1.6.0.RC2 (ruby 1.8.7 patchlevel 330) (2011-02-09 5434c72) has three tests failing, but this is the same as on master, so I'm guessing this has nothing to do with any_instance.

rubinius 1.2.3dev (1.8.7 0f984dae yyyy-mm-dd JI) [x86_64-apple-darwin10.6.0] is failing bundle install - linecache fails to build. Seems to be similar to rspec/rspec-core#260 but we need to use the rbx-linecache gem maybe?

Is there a CI server running builds against all supported rubies that I could refer to? If there isn't, I'd be happy to set one up.

[dchelimsky]

@kaiwren - we'll also need to support should_receive.

[kaiwren]

@dchelimsky - is there a safe use case for that? any_instance with stubs alone is already a smelly approach with limited application. I'm working on adding should_receive anyway, but let me know if you really want it in.

[dchelimsky]

I think one common case for any_instance is controller specs in Rails where users want to specify that a message is received by a model object. Because ActiveRecord hasn't offered identity map yet, we have to do things like this:

account = mock_model(Account, :id => "37")
Account.stub(:find).with("37") { account }
account.should_receive(:close)
post :close, :account_id => "37"

For many, the fact that this example might fail if we change the implementation in the controller action is a problem. While I agree that it is brittle, I don't mind it at all. It would only fail if I'm changing the code that it focuses on - not if I change model validations or something in the view - and the fact that it fails when I change the code gives me confidence that the example is testing what I think it's testing.

Also, once identity maps are released, we'll be able to slim that down to this:

account = Factory(:account)
account.should_receive(:close)
post :close, :account_id => account.id

But I digress.

A less brittle approach using any_instance would look like this:

account = Factory(:account)
Account.any_instance.should_receive(:close)
post :close, :account_id => account.id

This, of course, comes with its own risk that it is too loosely coupled to the code, but you'd have to work pretty hard to get this to pass without doing the right thing, so that risk is fairly low. Regardless, this is the sort of case I think people will want to use any_instance.should_receive for.

[kaiwren]

Fair enough. I've got should_receive working in c42engineering/rspec-mocks@0a864f5. Please take a look at the spec at line 80 - I based it on the specs in partial_mock_spec.rb but am unsure why the assertion that RSpec::Mocks::MockExpectationError will be raised is failing.

[kaiwren]

I've merged in alindeman's fixes from alindeman/rspec-mocks@9f82d0e and switched to evaling a string instead of using define_method - we're now green on 1.8.6, 1.8.7 and 1.9.2. I just need to add all the other should_receive stuff like exactly, once, twice etc. and we should be good to go.

The only thing that I still need help on is getting the two specs that use
expect{ klass.new.rspec_verify }.to raise_error(RSpec::Mocks::MockExpectationError)
and are currently marked pending to work as expected.

[kaiwren]

Done in c42engineering/rspec-mocks@7979521

[alindeman]

Awesome!

[kaiwren]

All done - see #46

[kaiwren]

Add support for any_instance:

• MyClass.any_instance.stub(:m)
• MyClass.any_instance.should_receive(:m)
• includes stub and mock APIs including
  • and_return, and_raise, and_yield
  • once, twice, exactly, any_number_of_times, never, at_least, at_most
• Closed by 8e68513.
• Closed by 8e68513.

[kaiwren]

My apologies for the back and forth, but I was pairing with a colleague on this issue and we noticed something interesting. We then validated with mocha and this is what we found:
require "spec_helper"

describe "any_instance in mocha" do
  it "should bar" do
    Array.any_instance.stubs(:foo).returns(1)
    [].foo.should eq(1) # Every instance of Array seems to be
    [].foo.should eq(1) # stubbed (as one would expect)
  end

  it "should foo" do
    Array.any_instance.expects(:foo)
    [].foo
    [] # In contrast, this line doesn't fail like I would expect it to
  end
end

Long story short, I see two distinct approaches for stubs and mocks in mocha conflated under the 'any_instance' banner. The approach used with stubs would have been better named 'every_instance' and the one used for expectations 'at_least_one_instance'.

So my question is this: do we want expectations in RSpec to also apply to every instance and not just at least one, like mocha?

[kaiwren]

Just to close the loop - this was fixed in #48.

[dchelimsky]

Hey guys - got one more bug: #54

I'm quite certain there will be more, as there is a lot of rich functionality that users will expect to work the same way on any_instance as it does on a specific instance.

[kaiwren]

@dchelimsky Cool, I'll look into it and get it fixed.

[kaiwren]

@dchelimsky - I didn't have much time this weekend, but I've made a minor change splitting any_instance into multiple files. Do let me know if this makes sense: https://github.com/c42engineering/rspec-mocks/tree/any-instance6