New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support stubs on any instance of a class #10

Closed
dchelimsky opened this Issue Aug 11, 2010 · 37 comments

Comments

Projects
None yet
4 participants
@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Aug 11, 2010

Contributor

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

Contributor

kaiwren commented Aug 11, 2010

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

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Aug 11, 2010

Member

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

Member

dchelimsky commented Aug 11, 2010

@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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Aug 11, 2010

Contributor

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.

Contributor

kaiwren commented Aug 11, 2010

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Aug 12, 2010

Contributor

'#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?

Contributor

kaiwren commented Aug 12, 2010

'#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

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Aug 12, 2010

Member

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?

Member

dchelimsky commented Aug 12, 2010

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Aug 12, 2010

Contributor

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

Contributor

kaiwren commented Aug 12, 2010

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Aug 12, 2010

Contributor

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

Contributor

kaiwren commented Aug 12, 2010

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Aug 12, 2010

Contributor

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.

Contributor

kaiwren commented Aug 12, 2010

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

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Aug 17, 2010

Member

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

Member

dchelimsky commented Aug 17, 2010

@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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Aug 17, 2010

Contributor

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

Contributor

kaiwren commented Aug 17, 2010

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

@oriolgual

This comment has been minimized.

Show comment
Hide comment
@oriolgual

oriolgual Aug 29, 2010

Any news on when this will be merged to master?

Thanks for your hard work!

oriolgual commented Aug 29, 2010

Any news on when this will be merged to master?

Thanks for your hard work!

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Aug 29, 2010

Member

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

Member

dchelimsky commented Aug 29, 2010

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

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Sep 1, 2010

Contributor

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

Contributor

kaiwren commented Sep 1, 2010

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

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Mar 9, 2011

Contributor

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?

Contributor

alindeman commented Mar 9, 2011

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 9, 2011

Contributor

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.

Contributor

kaiwren commented Mar 9, 2011

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

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Mar 9, 2011

Member

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

Member

dchelimsky commented Mar 9, 2011

@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

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Mar 9, 2011

Contributor

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

Contributor

alindeman commented Mar 9, 2011

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

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 9, 2011

Contributor

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.

Contributor

kaiwren commented Mar 9, 2011

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

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Mar 9, 2011

Contributor

Maybe investigate how mocha achieves it?

Contributor

alindeman commented Mar 9, 2011

Maybe investigate how mocha achieves it?

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 14, 2011

Contributor

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

Contributor

kaiwren commented Mar 14, 2011

@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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 15, 2011

Contributor

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.

Contributor

kaiwren commented Mar 15, 2011

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

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Mar 15, 2011

Member

Looks good to me. Please proceed.

Member

dchelimsky commented Mar 15, 2011

Looks good to me. Please proceed.

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 15, 2011

Contributor

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.

Contributor

kaiwren commented Mar 15, 2011

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

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Mar 15, 2011

Member

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

Member

dchelimsky commented Mar 15, 2011

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

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 16, 2011

Contributor

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

Contributor

kaiwren commented Mar 16, 2011

@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

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Mar 16, 2011

Member

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.

Member

dchelimsky commented Mar 16, 2011

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 16, 2011

Contributor

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.

Contributor

kaiwren commented Mar 16, 2011

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 21, 2011

Contributor

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.

Contributor

kaiwren commented Mar 21, 2011

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren
Contributor

kaiwren commented Mar 21, 2011

@alindeman

This comment has been minimized.

Show comment
Hide comment
@alindeman

alindeman Mar 21, 2011

Contributor

Awesome!

Contributor

alindeman commented Mar 21, 2011

Awesome!

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 23, 2011

Contributor

All done - see #46

Contributor

kaiwren commented Mar 23, 2011

All done - see #46

@dchelimsky dchelimsky reopened this Mar 23, 2011

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 24, 2011

Contributor

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

kaiwren commented Mar 24, 2011

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.

@dchelimsky dchelimsky closed this Mar 24, 2011

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Mar 25, 2011

Contributor

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?

Contributor

kaiwren commented Mar 25, 2011

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Apr 8, 2011

Contributor

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

Contributor

kaiwren commented Apr 8, 2011

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

@dchelimsky

This comment has been minimized.

Show comment
Hide comment
@dchelimsky

dchelimsky Apr 21, 2011

Member

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.

Member

dchelimsky commented Apr 21, 2011

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

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren Apr 22, 2011

Contributor

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

Contributor

kaiwren commented Apr 22, 2011

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

@kaiwren

This comment has been minimized.

Show comment
Hide comment
@kaiwren

kaiwren May 1, 2011

Contributor

@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

Contributor

kaiwren commented May 1, 2011

@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

kaiwren added a commit to c42engineering/rspec-mocks that referenced this issue Jun 12, 2011

myronmarston pushed a commit to myronmarston/rspec-mocks that referenced this issue Oct 21, 2012

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

- Closes #46.
- Closes #10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment