Allow for using class instance in block passed to stub method #236

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@sheerun
sheerun commented Mar 6, 2013

It allows to use and test mocked object in block
passed to stub or using_self method (among others)

It is especially useful when stubbing #all_instances

@sheerun sheerun Add MessageExpectation#using_self method
It allows to use and test mocked object in block
passed to stub or using_self method (among others)

It is especially useful when stubbing #all_instances
65ca51e
@sheerun
sheerun commented Mar 6, 2013

Use case:

Hash.any_instance.should_receive(:encode_json).using_self do
  self['model'].should == @model.serialized_hash
end.exactly(:once)

Advanced usage:

ActiveRecord::Base.any_instance.should_receive(:as_json).using_self do |options|
  options[:root].should be_present
  { self.class.to_s.downcase => "Stub!" }
end

You can use yield and use &block in implementation as usual and even proxy to old method:

Net::HTTP.any_instance.should_receive(:get).using_self do |options, &block|
  proxy?.should == true
  get(options, &block)
end
@myronmarston
Member

@sheerun -- great work on this! It's always nice to see new features coming from contributors :).

I don't understand your use case example...given that you've set a message expectation on Hash.any_instance, wouldn't self.class be Hash, not String? Or am I misunderstanding something?

Anyhow, you've taken a quite different (but interesting) approach here to what I had in mind. Originally I was thinking it would work like this:

# normally, this is how it works...
MyClass.any_instance.stub(:foo) do |arg_1, arg_2, &block|
end

# but if you change the config...
RSpec::Mocks.configure do |c|
  c.yield_instance_to_any_instance_implementation_blocks = true
end

# ...then the yielded args change to:
MyClass.any_instance.stub(:foo) do |my_class_instance, arg_1, arg_2, &block|
end

I think the implementation of this approach would be much, much simpler: no need to mess with defining a method dynamically, or eval'ing the block in a different context or anything...we just add a simple conditional that checks the value of the config option, and, if it is true, prepends the yield args list with the object instance before calling the implementation block.

On top of that, I think it could be potentially troublesome to change the eval context in the implementation block like that. It's a much bigger change (as you suddenly can't call helper methods defined in the example group like you could before) compared to just yielding an extra arg; plus it's more inline with how block implementations work elsewhere in rspec-mocks.

Thoughts?

@sheerun
sheerun commented Mar 7, 2013

Uh. I respond once again, because I didn't read you carefully. I don't really like this approach, because it changes arguments order in every already implemented implementation block, so you need to redefine every single one of them (if one defined a block accepting |foo, bar|, then foo would become self and bar would become foo).

If anything, class instance should be passed as the last argument but that's also troublesome because one could test for arguments count or use variadic arguments (I bet it would break specs in many other places too).

With my implementation the problems described above don't exist and you can set behavior in per-spec, not global way. It won't break any previous specs because there is no difference in context if you don't use using_self method. You can use any .should matchers even in the changed context and if you have to use methods in rspec context, you can always use lexical scoping:

rspec = self
Bar.any_instance.stub(:bar).using_self do
  rspec.expect(self).to eq(@bar)
end

I would discuss the name of this method and the exact behavior, but in my opinion this solution is generally ok.

I also fixed and extended use cases.

@cupakromer
Member

I find the use of the self in the block to be very surprising. I would not expect the block to change my current context. I would much rather have that instance object passed into the block. With that in mind, the configuration route feels like it would probably break too many existing tests. I would probably only want this to happen some of the time, not globally.

What about splitting the difference, something like:

describe "a stubbed implementation" do
  it "works with return values" do
    Object.any_instance.stub(:foo).on_instance do |instance, arg|
      if arg == :this
        instance
      elsif arg == :that
        "got that"
      end
    end

    object = Object.new

    expect(object.foo(:this)).to be(object)
    expect(object.foo(:that)).to eq("got that")
  end

  it "works within the current context" do
    actual = nil
    Object.any_instance.stub(:foo).on_instance do |instance|
      actual = instance
    end

    object = Object.new
    object.foo

    expect(actual).to be(object)
  end
end

I've been going back and forth with the name on_instance and using_instance, and don't really like either.

@sheerun
sheerun commented Mar 7, 2013

That's also good idea. Maybe use each_instance instead if any_instance (or something similar), because I cannot find use case when the any_instance is not used..

@myronmarston
Member

@cupakromer -- thanks for helping us figure this out!

I'm not opposed to making it configurable on a per-stub basis; however, I do have a reason to prefer it being global...

In my opinion, we should have included the object instance in the yielded args from the very start. We didn't think things through to realize that without this, users have no way to get a reference to the stubbed object. Given that, I'd like to move in the direction of making the object instance always be yielded from the any_instance block implementations. We can change the spec_helper.rb generate by rspec --init so that it turns this on by default for new projects, and we can consider making it default to on in some future major release (e.g. maybe RSpec 3 or 4), while still allowing folks to disable it as needed. Making it a global config is far simpler than adding to the fluent interface, and essentially "corrects" the oversite that caused us to not include the object instance in the first place.

It's true that folks who want to use this feature would need to turn on the config option, and go through and fix all their existing any_instance block implementations, but that doesn't bother me too much...we're providing a new feature that users have to opt-in to use on an existing project, and I would expect it to be pretty easy to use an editor macro to go through a fix all uses of any_instance block implementations in a short time period.

BTW, we briefly discussed a similar addition to the fluent interface to configure what args a particular block implementation is yielded, but decided against it for various reasons. The discussion is here:

#23 (comment)

@sheerun
sheerun commented Mar 7, 2013

ok, I agree

@cupakromer
Member

Ah, good explanation of the history and direction you plan on going. 👍

@cupakromer
Member

@myronmarston should we close this in favor of #175? Or vice versa? Seems they are the same request just different suggested implementation approaches.

@myronmarston
Member

Thanks for helping triage stuff, @cupakromer. I think we're not going to merge this solution to the issue for the reasons I've stated above, so I'm going to close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment