Add method to call_original method from a stub #23

Merged
merged 11 commits into from Oct 26, 2012

Conversation

Projects
None yet
Owner

myronmarston commented Oct 21, 2012

foo.stub(:bar) do
  # do something relevant to the example
  call_original
end

foo.stub(:bar) do |*args|
  # do something relevant to the example
  call_original(*args)
end
Contributor

txus commented Oct 18, 2010

Just out of curiosity, how could this be implemented? I mean, in which scope should call_original be declared?

Owner

dchelimsky commented Oct 18, 2010

I think we'll need to define a new scope. Right now the block is invoked via block.call.

Contributor

txus commented Oct 27, 2010

I've started working on this, changing the context in which the block is called. Nevertheless, by doing that, you can't use expectations inside a return block, since rspec-mocks does not know anything about rspec-expectations. If we want to avoid coupling between expectations and mocks, I can't see a way this could be solved. What do you think?

http://github.com/txus/rspec-mocks/commit/362c9506f794e67f4b03b7902c096bbe27bd5e9d

Contributor

dnurzynski commented Jan 17, 2011

I've pulled txus commit and change method implementation to calls block in current block context clone. When I tried to do this different way there were always some issues with current block context but as txus said we should avoid dependencies.

commit: dnurzynski/rspec-mocks@b109f24

branch: https://github.com/dnurzynski/rspec-mocks/commits/call_original_feature

( pull requests has better view but not sure to use it because of double issues ? )

jbrains commented Jan 5, 2012

I'd like to keep an eye on this. I want to be able to set an expectation on all calls in a recursive method call chain except the first one. This would let me verify that an ERb template attempts to render its partials.

Owner

myronmarston commented Jun 27, 2012

FWIW, you can get this behavior today with a bit of extra work:

original = foo.method(:bar)
foo.stub(:bar) do |a, b|
  do_something_with(a, b)
  original.call(a, b)
  do_something_else_with(a, b)
end
Owner

myronmarston commented Aug 4, 2012

I've been thinking more about this. The proposed API (call_original) is great, but all of the proposed solutions involve metaprogramming tricks that I'm uncomfortable with--not because I don't understand what's going on (I do) but because it has the potential to cause problems for end users. Currently, there are 3 ways this could work:

  • Eval the implementation block in a different context that has call_original available. This is what @txus's solution is doing, if I read it right. The problem is, instance_eval'ing the implementation block can create all sorts of confusion for end users because self in the block will be different. It's not how it works now and I think we should avoid it.
  • Define #call_original on the object that was self at the point of block definition. This is what @dnurzynski's solution does. I think this has less potential for problems than the first solution, but with this approach we're adding methods to objects we don't own. There's a time and place for that but I would consider it a solution of last resort.
  • Pass the block an additional argument which is the original method; then the user can use original.call(*args) wherever they like in their block. However, it would be a massive breaking change if we suddenly started yielding an extra argument to the block.

Of these, I think the last solution would have the fewest problems if we could find a way to avoid it being a massive breaking change. To that end, two thoughts:

  • We could introduce a config option (e.g. pass_original_method_to_implementation_blocks or something like that) that toggles this behavior.
  • We've been discussing a potential new syntax in #153. If/when we add that, we could have that pass the extra original method arg to all block implementations.

One last thought: I think the most common use cases for this are injecting some logic before or after the original method when it is called. We could support these with stub_before and stub_after methods:

foo.stub_before(:bar) do |*args|
end

foo.stub_after(:bar) do |*args|
end

With these, there's no need for call_original because it is implicit that it the original will be called after the given block (for stub_before) or before the given block (for stub_after). This wouldn't handle the "stub around" case, though.

Thoughts?

Owner

dchelimsky commented Sep 9, 2012

I'd prefer not to add more methods on object to support this behavior, and I agree we should not break existing behavior. What about a new API like this:

intercept(foo, :bar) do |real_object, *args|
  do_some_stuff_before
  real_object.bar(*args)
  do_some_stuff_after
end
Owner

myronmarston commented Sep 10, 2012

That's a pretty good API, but making that work would be quite difficult. Consider that the real_object.bar(*args) within the intercept block should not be intercepted (or we'd have infinite recursion)--so we'd have to wrap the calling of the block itself in some kind of messy without_interceptions { } mechanism.

On top of that, that looks like it only works for stubs, not for mocks--i.e. I don't see how intercept supports expecting #bar will be received on foo.

Given that we're already talking about adding a new syntax (#153), it's a great opportunity to add support for this w/o breaking existing tests and without needing an alternate API just for this. Conceptually, I think it makes a lot of sense to yield the original method object to block implementations for either a stubbed or mocked method. Being able to call the original would then fall naturally out of that.

As an example, if we decide to go with the on syntax we've discussed in #153, and added the extra block arg to support this, the code would be like so:

on(foo).stub(:bar) do |orig_bar, *args|
  do_some_stuff_before
  orig_bar.call(*args)
  do_some_stuff_after
end

Actually, one more idea that just popped in my head:

foo.stub(:bar).with_original_method do |orig_method, *args|
  do_some_stuff_before
  orig_method.call(*args)
  do_some_stuff_after
end

Essentially, we add an additional method to the fluent interface that explicitly tells rspec-mocks to yield the original method object to the block implementation. We could add this w/o breaking anything, but it would be a bit verbose.

I'm not 100% sure of the value of self in these examples so bear with me. A few ideas:

foo.stub(:bar) do
  # do something relevant to the example
  original.call
end
foo.stub(:bar) do
  # do something relevant to the example
  @_original.call
end

Those are both kind of magic though so I don't really like them. Shorter than other things thrown out there though...

foo.stub(:bar) do
  foo.stub.original(:bar).call
end

This extends the existing API so maybe you won't like it. I kind of do though because it lets us get at the sub object in a way. That said now the API maybe gets a bit wonky bc sometimes stub stubs something, other times it gets you access to the stub where you can get the original (and presumably not much else at this point). But maybe you like how it reads.

(I'm unsure of whether to use a call to #call or not...I prefer it because it's consistent with Ruby's block / method API, but I could see people thinking that foo.stub.original(:bar) might invoke the method. Then again, somebody doing this sort of stuff is probably more advanced and gets the #call syntax?

Owner

myronmarston commented Sep 11, 2012

We currently don't change the value of self in the block implementations, so it would generally be the RSpec::Core::ExampleGroup instance that the example is eval'd in. Your first two ideas seem to depend upon self being the stubbed object, so it's probably not going to work unless we instance_eval the block implementation but I want to avoid that (due to the confusion that arises when you change self).

The foo.stub.original(:bar).call idea is interesting. It'd certainly work without the instance_eval or monkey patching gymnastics that some of the original ideas above required. Not sure I like how it reads, though.

Re: #call: I think I'd like the API to provide the original method object and allow users to use call. As you point out, it falls naturally out of ruby. Any experienced rubyist would expect this API, I think, and users who don't know about method objects need to learn, anyway :).

Contributor

alindeman commented Sep 11, 2012

We could go a tad crazy in order to get the first two interfaces without all the terribleness of instance_eval:

class Foo
  def self.new_block
    @ivar = "inside foo"
    proc { "#{@ivar} #{call_original}" }
  end
end

blk = Foo.new_block

blk_binding = blk.binding
blk_binding.eval "def self.call_original; 'returned from call_original'; end"

puts blk.call # "inside foo returned from call_original"

# ... remove call_original method from blk_binding ...

I'm obviously unconvinced because I'd never write this kind of code in a production app; however, we are in a mocking library where the rules seem a bit different :)

Owner

myronmarston commented Sep 11, 2012

@alindeman -- that's basically what @dnurzynski's solution above does.

I'm uncomfortable doing this, even though this is a mocking library that does highly meta stuff. Yielding the original method as a block arg seems so much simpler to me.

Contributor

alindeman commented Sep 11, 2012

I'm uncomfortable doing this, even though this is a mocking library that does highly meta stuff. Yielding the original method as a block arg seems so much simpler to me.

I think I agree. I think my favorite so far is the chained with_original_method.

Owner

myronmarston commented Sep 11, 2012

I think I agree. I think my favorite so far is the chained with_original_method.

If backwards compatibility weren't a concerned, would you still prefer this over simply always yielding the original method to the implementation block as the first argument?

I like the with_original_method idea OK but mostly because it seems necessary to keep backwards compatibility. If we go with one of the new syntaxes discussed in #153, I think I'd prefer to just make the implementation blocks always receive the method arg when used with that syntax.

Contributor

alindeman commented Sep 12, 2012

If backwards compatibility weren't a concerned, would you still prefer this over simply always yielding the original method to the implementation block as the first argument?

My gut reaction is yes I would still prefer it because calling through to the original method seems like the less used case. In general when I'm stubbing, I don't need to call through. Is it more common than I think though?

I feel like the backwards compatibility case is worth considering too. Even if it were made in a major release bump, I feel like breaking changes should still be made very carefully. It seems like it might be frustrating to a lot of folks if the behavior of stub with a block changed in what arguments were yielded.

That said, if calling through is more common than I seem to think, I'm willing to be convinced.

Owner

dchelimsky commented Sep 12, 2012

My gut reaction is yes I would still prefer it because calling through to the original method seems like the less used case. In general when I'm stubbing, I don't need to call through. Is it more common than I think though?

It's uncommon now because there is no support for it yet :)

When I first heard the term "spy", I incorrectly intuited it to mean "observe when this method is called, but don't replace it." That's the primary impetus for this feature.

Owner

myronmarston commented Sep 12, 2012

I feel like the backwards compatibility case is worth considering too. Even if it were made in a major release bump, I feel like breaking changes should still be made very carefully. It seems like it might be frustrating to a lot of folks if the behavior of stub with a block changed in what arguments were yielded.

Absolutely. I'm not advocating breaking backwards compatibility. I can see I didn't explain myself very well.

What I'm suggesting is that if we introduce a new rspec-mocks syntax, it affords us the opportunity to introduce other changes like these without any risk of breaking backwards compatibility because no one is using the new syntax yet.

So, if we went with the expect/allow suggestion from that thread, you could do:

expect(foo).to receive(:bar) { |orig_bar, some_arg|
  something_before_original
  orig_bar.call(some_arg)
  something_after_original
end

...but a block implementation with the existing syntax would not receive the method object:

foo.stub(:bar) do |some_arg|
  # no way to call original
end

This would not break backwards compatibility. If we wanted to support this feature with the old syntax, the with_original_method chained method could be used when you're using the old syntax.

My gut reaction is yes I would still prefer it because calling through to the original method seems like the less used case. In general when I'm stubbing, I don't need to call through. Is it more common than I think though?

If you don't need to call through, it doesn't cause any problem to receive the extra block arg. It can simply be ignored.

I can see that the logic in this discussion is mostly about decorating a method, so would it be more appropriate to name it decorate?

Additionally, I would like to suggest an alternative syntax for the simpler case where the user would like to stub the method for particular inputs, and otherwise fallback to the original behavior. I am thinking of something along the following lines:

def stub_with_fallback(obj, method)
  original_method = obj.method(method)
  obj.stub(method).with(anything()) { |*args| original_method.call(*args) }
  return obj.stub(method)
end

# usage example:
stub_with_fallback(File, :exist?).with(/txt/).and_return(true)

(Code copied from my answer on StackOverflow)

It should be simple to provide such a method while keeping backward compatibility. What do you think?

Owner

myronmarston commented Sep 21, 2012

I like decorate quite a bit. My main concern there is that there are cases where you want to do this with a stub and other cases where you want to do it with a mock...and having one method (decorate) doesn't make it easy to distinguish without some messy options. I'd prefer to integrate it directly in the main mocking/stubbing syntax.

rkh commented Oct 18, 2012

I would also like something along the lines of:

something.should_receive(:foo).and_call_original

This would allow just checking invokation without ending up in an seemingly endless chain of mocks.

Owner

myronmarston commented Oct 18, 2012

I'm going to try to get this in the next release. Here's the direction I'm planning on taking...

  • yield_original_method will be added to the fluent interface. When you provide a block implementation after calling this, the block will receive the original method object as the first argument. This allows you to do things like change the arguments before passing them on to the original implementation. At first, I was planning on calling this with_original_method, but we already have with on the fluent interface and this means something quite different.
  • and_call_original will be added for @rkh's case.

One question: what should it do if yield_original_method is used but no block implementation is provided? Maybe yield_original_method should require a block, and should raise an error at the point it's called if none is provided? If we go that direction, this would work:

obj.stub(:bar).with(3).yield_original_method { }

...but this would raise an error:

obj.stub(:bar).yield_original_method.with(3) { }

...which I think is OK. Thoughts?

One other thing I'm toying with: adding a config option that would make rspec-mocks always yield the original method to block implementations. It'll default to off for backwards compatibility, and yield_original_method is provided for that case so that you can use it on one stub w/o needing to change the global config for the whole project and potentially break things. In RSpec 3 we can consider defaulting it to on.

Good idea or bad idea?

Owner

dchelimsky commented Oct 18, 2012

@myronmarston I actually think yield_original_method is going to be very confusing for ppl and is probably not necessary if we have and_call_original (which solves the real problem IMO).

Owner

myronmarston commented Oct 18, 2012

and_call_original is perhaps the common case, but it's far less flexible than yield_original_method would be. I've had cases in the past where having yield_original_method would have come in handy, but I've been able to work around it using something like:

orig_method = obj.method(:foo)
obj.stub(:foo) do |arg|
  if arg == :blah
    17
  else
    orig_method.call(arg)
  end
end

...so I've gotten by without before now, but I like the full flexibility yield_original_method would afford. Why do you think it would confuse people?

Owner

dchelimsky commented Oct 18, 2012

Because it's a different block API - other uses of the block only get the submitted args as block args, whereas this one method gets an additional block arg. To me, that violates principle of least surprise.

Owner

myronmarston commented Oct 18, 2012

Because it's a different block API - other uses of the block only get the submitted args as block args, whereas this one method gets an additional block arg. To me, that violates principle of least surprise.

Hmm, I get your point, although given that the method is explicitly saying "yield the original method, too", it doesn't seem like it would be surprising (you've just told rspec-mocks to do that....).

What do you think about the config option instead?

Owner

dchelimsky commented Oct 18, 2012

Hmm, I get your point, although given that the method is explicitly saying "yield the original method, too"

But it also yields the args, not just the original method, right? So the name doesn't really tell the whole story, and I don't want to type 'yield_original_method_and_submitted_args'.

Personally, I think the last example you gave as a workaround is perfectly expressive and we should document that and recommend it as a solution to that problem.

myronmarston was assigned Oct 20, 2012

Owner

myronmarston commented Oct 21, 2012

I just pushed a first-pass implementation for this. A few random thoughts/questions:

  • To get this to work, I had to change the MessageExpectation initializer so that it receives the MethodDouble as an argument. This was necessary so that and_call_original could query the method double for the original method. This increases the coupling between MessageExpectation and MethodDouble a bit (MessageExpectation used to know nothing about MethodDouble). Anyone got any ideas how to achieve this behavior w/o increasing this coupling?
  • This turned out to be more complex than I expected, so I commented MethodDouble#original_method pretty heavily (even though I usually favor self-explanatory code that needs no comments). Is this overkill or helpful?
  • There are a couple different ways we can handle method_missing. On 1.9, w/ a well-behaved object that defines respond_to_missing?, we're able to get a method object for a method_missing-handled message, but in other cases I thought it still valuable to try to handle the method_missing case as close to how the original object does as possible. If the object does not define method_missing I have it giving early "your object does not implement that message" feedback from and_call_original, but otherwise, I try to use method_missing in case the object does handle the message. The result is that there is a slight inconsistency: if an object defines method_missing but doesn't handle a particular message (e.g. because it supers for particular messages), and_call_original will not complain, and the user will get a NoMethodError when the message is received...but for an object that does not define method_missing, they'll get an immediate error when and_call_original is called. Is this OK or is there a better behavior here?
  • I think that and_call_original only makes sense on partial mock objects, so I made it raise an error when used on a pure mock object, even if it's a message the mock object responds to (e.g. object_id, to_s or something basic like that).

Feedback wanted!

Owner

myronmarston commented Oct 21, 2012

I found something very, very odd...on 1.8.7, I got a strange failure when the specs ran in a particular order. For some reason, declaring a test double and calling should be_null_object on it changes the class of error that gets raised from NoMethodError to NameError....which makes absolutely no sense to me. I copied the snippet from another test over to consistently trigger the bug but I don't plan to merge that commit.

Contributor

alindeman commented Oct 21, 2012

I've seen something similar in Rubinius, but can't be sure it's definitely related: rubinius/rubinius#1938

Owner

myronmarston commented Oct 21, 2012

I've seen something similar in Rubinius, but can't be sure it's definitely related: rubinius/rubinius#1938

I thought about that because I remember you finding that bug. But this is 1.8.7 so I don't see how it can be related to a rubinius bug.

Another weird thing about it: it's a heisen bug. The act of stepping through it in a debugger changes it so that it passes. I'm starting to think I don't understand ruby or how computers work :-(.

If someone else wants to dig into it, feel free, otherwise I'll probably just change the spec so that it expects NameError (the superclass of NoMethodError) so that it passes regardless.

@alindeman alindeman and 1 other commented on an outdated diff Oct 21, 2012

lib/rspec/mocks/method_double.rb
@@ -41,6 +41,53 @@ def visibility
end
# @private
+ def original_method
+ if @stashed_method.method_is_stashed?
+ # Example: a singleton method defined on @object
+ @object.method(@stashed_method.stashed_method_name)
@alindeman

alindeman Oct 21, 2012

Contributor

We may want to use:

::Object.instance_method(:method).bind(@object).call(@method_name)

to avoid problems with folks overriding #method (either accidentally or on purpose)

@myronmarston

myronmarston Oct 21, 2012

Owner

Good idea...I remember there was a place you did that previously. I think we may want to look into extracting that into something common (like a mixin) so we can just say method_from(@object, method_name), which is more expressive/less noisy, plus it could potentially be faster by caching the result of ::Object.instance_method(:method) since that is essentially immutable. (Although a user could of course redefine it but you'd have to be crazy to do so and all bets are off when you are doing invasive things like that).

@alindeman alindeman and 1 other commented on an outdated diff Oct 21, 2012

lib/rspec/mocks/method_double.rb
@@ -41,6 +41,53 @@ def visibility
end
# @private
+ def original_method
+ if @stashed_method.method_is_stashed?
+ # Example: a singleton method defined on @object
+ @object.method(@stashed_method.stashed_method_name)
+ else
+ begin
+ # Example: an instance method defined on @object's class.
+ @object.class.instance_method(@method_name).bind(@object)
@alindeman

alindeman Oct 21, 2012

Contributor

I think we could use

::Object.instance_method(:method).bind(@object).call(@method_name)

here too. Though I think your code here should be functionality equivalent?

@myronmarston

myronmarston Oct 21, 2012

Owner

What I have here and what you suggest are two different things. Consider a case like this:

class A
  def b
  end
end

describe A do
  specify do
    a = A.new
    a.should_receive(:b).and_call_original
  end
end

a.should_receive(:b) has the affect of defining a singleton method on a to do the mocking. This method is what your expression above would return (which is not the original method). In contrast, the expression I have above gets the b method as it was defined as an instance method on A, and then it binds it to a...which is the original method before should_receive defined the singleton method.

@alindeman alindeman and 1 other commented on an outdated diff Oct 21, 2012

lib/rspec/mocks/method_double.rb
+ # Example: an instance method defined on @object's class.
+ @object.class.instance_method(@method_name).bind(@object)
+ rescue NameError
+ raise unless @object.respond_to?(:superclass)
+
+ # Example: a singleton method defined on @object's superclass.
+ #
+ # Note: we have to give precedence to instance methods
+ # defined on @object's class, because in a case like:
+ #
+ # `klass.should_receive(:new).and_call_original`
+ #
+ # ...we want `Class#new` bound to `klass` (which will return
+ # an instance of `klass`), not `klass.superclass.new` (which
+ # would return an instance of `klass.superclass`).
+ @object.superclass.method(@method_name)
@alindeman

alindeman Oct 21, 2012

Contributor

If we used Object.instance_method(:method).bind(@object).call(@method_name), would we need this case?

@myronmarston

myronmarston Oct 21, 2012

Owner

Yes, I think so. Singleton methods on class objects act different from singleton methods on non-class objects in that singleton methods on classes can apply to other objects (specifically subclasses), so when the object is a class, we need to look to the superclass to find the method we want.

@justinko justinko and 3 others commented on an outdated diff Oct 21, 2012

lib/rspec/mocks/method_double.rb
@@ -41,6 +41,53 @@ def visibility
end
# @private
+ def original_method
+ if @stashed_method.method_is_stashed?
@justinko

justinko Oct 21, 2012

Contributor

Isn't this incorrect semantics? How can a "stashed method" be asked if it is stashed?

@myronmarston

myronmarston Oct 22, 2012

Owner

It's an instance of RSpec::Mocks::StashedMethod which conditionally stashes a method only if its needed.

I think that you're right that the class and variable are a bit misnamed--the name implies it is always stashed when that is not the case (which is why we check method_is_stashed? here).

@alindeman -- you came up with the RSpec::Mocks::StashedMethod abstraction originally...what do you think about renaming it to MethodStasher? That makes it clear that it's role is to stash methods without implying that it always does so.

@alindeman

alindeman Oct 22, 2012

Contributor

Makes sense to me! At the time, I was trying to keep as much of the "does this method need to be stashed?" knowledge in one place (as it got more complicated with the fix I was implementing); this new feature seems like a good reason to change things around though.

@linhchauden

linhchauden Oct 23, 2012

Somewhere inbetween?

Owner

myronmarston commented Oct 22, 2012

Thanks for the feedback, @alindeman / @justinko.

I'm still interested in feedback about one of the primary choices I had to make for this....how to handle method missing. I realize what I wrote above was probably not very clear, so here's a more detailed explanation of the issue.

Consider the following three classes (and corresponding instances):

class A
end

a = A.new

class B
  def method_missing(name, *args)
    return super unless name.to_s =~ /^greet_/
    :greet
  end
end

b = B.new

class C
  def respond_to_missing?(name)
    name.to_s =~ /^greet_/ || super
  end

  def method_missing(name, *args)
    return super unless name.to_s =~ /^greet_/
    :greet
  end
end

c = C.new

Now consider these 5 cases:

  1. a.should_receive(:undefined).and_call_original
  2. b.should_receive(:greet_me).and_call_original
  3. b.should_receive(:undefined).and_call_original
  4. c.should_receive(:greet_me).and_call_original
  5. c.should_receive(:undefined).and_call_original

Cases 2 and 4 involve messages the objects will respond to. The other cases don't. However, the only case where #method(method_name) will give us a method object is case 4 (that's one of the things defining respond_to_missing? gets you on 1.9).

So, the question is, how should we handle the other 4 cases? We can create a method-like object like so:

Proc.new do |*args, &block|
  object.method_missing(method_name, *args, &block)
end

Just like a real method object, when call is sent to this object, the object will respond just as it did originally...which may include getting a NoMethodError.

In general, I think early feedback is important, and if we can give users a reasonable "the object does not implement #blah" error when and_call_original is called for a message the object does not respond to, that seems preferable to waiting until users send the expected message, at which time they'll get a NoMethodError. However, we have no way to tell whether or not b will respond to a particular message -- so we can either create our faux method object (which allows case 2 to work, but provides a late NoMethodError error for case 3) or we can give an early "does not implement error" for both cases.

For the first case, we can actually tell that a does not respond to :undefined, because a.method(:method_missing).owner == Object.instance_method(:method_missing).owner -- which tells us a's only method_missing definition is the root one.

So...here's how things currently work with the code in this PR:

  • Cases 2 and 4 will both work.
  • For cases 3 and 5 the user will get a NoMethodError when the expected message is sent to the object.
  • For case 1 the user will get a "the object does not implement that method" error when and_call_original is called.

This seemed like the best behavior to me yesterday, but it is kinda weird that it's a bit inconsistent: when an object does not respond to a message, sometimes the user gets an immediate error...and in other cases they get a later error when the message is sent. I could remove the early "the object does not implement..." error entirely but that seems to be a useful error, especially since it's for the common case of an object not defining method_missing.

So we're left with having to make a choice between the optimal behavior for each particular situation based on the information we have available (as things work now) and consistency. Which is more important?

Contributor

justinko commented Oct 22, 2012

Consistency. Always.

Contributor

justinko commented Oct 22, 2012

All or nothing at all.

Owner

dchelimsky commented Oct 22, 2012

I think it would be pretty obvious that a NoMethodError on foo when the example says obj.should_receive(:foo).and_call_original means the object doesn't handle foo, so I don't see much value in having a slightly different "the object does not implement ..." message - let the object respond the way it does in Ruby.

Also, one my reasons for avoiding rspec-fire-like behavior (with which this overlaps a bit) is that the method might not yet exist at declaration time and then come into existence via meta-programming before it is actually invoked, leading to potentially confusing behavior. To that end, I think the correct approach is to prefer the object's natural response at the time of invocation even if that means losing the benefit of failing slightly faster.

myronmarston referenced this pull request in rspec/rspec-expectations Oct 23, 2012

Closed

Add spec demonstrating odd `method_missing` behavior. #183

myronmarston added some commits Oct 23, 2012

@myronmarston myronmarston Remove `method_missing` inconsistency.
Based on conversations with @dchelimsky and others, we've decided it's best not
to raise an early error from `and_call_original` as that creates an
inconsistency since we sometimes don't know if `method_missing` will handle
the message or not.
98ca879
@myronmarston myronmarston Rename StashedInstanceMethod to InstanceMethodStasher.
The old name implied it always stashed a method (and made
`stashed_method.method_is_stashed?` read funny). The new
name makes it clear what role the object plays without
implying that it always stashes the method.
503c269
@myronmarston myronmarston Add additional yard private declarations. f6d03d5
Owner

myronmarston commented Oct 23, 2012

Thanks for the feedback!

As far as I know this is ready to merge...I'll probably merge tomorrow unless I hear differently. Please review the final changes if you get a chance.

@dchelimsky dchelimsky commented on an outdated diff Oct 24, 2012

spec/rspec/mocks/and_call_original_spec.rb
+ def self.new_instance
+ new
+ end
+ end
+ end
+
+ let(:instance) { klass.new }
+
+ it 'passes the received message through to the original method' do
+ instance.should_receive(:meth_1).and_call_original
+ expect(instance.meth_1).to eq(:original)
+ end
+
+ it 'passes args and blocks through to the original method' do
+ instance.should_receive(:meth_2).and_call_original
+ value = instance.meth_2(2) { |a, b| a * b }
@dchelimsky

dchelimsky Oct 24, 2012

Owner

This is a little hard to follow. What do you think about something like:

value = instance.meth_2(:submitted_arg) { |a, b| [a, b] }
expect(value).to eq [:submitted_arg, :additional_yielded_arg]

@dchelimsky dchelimsky commented on an outdated diff Oct 24, 2012

spec/rspec/mocks/and_call_original_spec.rb
+ end
+
+ it "works for instance methods defined on the object's class's superclass" do
+ subclass = Class.new(klass)
+ inst = subclass.new
+ inst.should_receive(:meth_1).and_call_original
+ expect(inst.meth_1).to eq(:original)
+ end
+
+ context 'on an object that defines method_missing' do
+ before do
+ klass.class_eval do
+ private
+
+ def method_missing(name, *args)
+ return super unless name.to_s =~ /^greet_(.*)$/
@dchelimsky

dchelimsky Oct 24, 2012

Owner

I think this could just be `== 'greet_jack' - no need for the complexity of the regex - wdyt?

myronmarston added some commits Oct 24, 2012

@myronmarston myronmarston Clarify specs based on @dchelimsky's feedback. aff31ae
@myronmarston myronmarston Remove code that should have been removed in 98ca879.
I forgot to remove this code as part of that commit.
d221e27
@myronmarston myronmarston Deal with 1.8 shortcoming.
1.8 does not allow you to bind a singleton method from a class's superclass to the class, so our original code failed. This tries to work around the issue while alerting the user to the fact that it may not work correctly in all cases.
fafb085
Owner

myronmarston commented Oct 25, 2012

So I found one more issue...some code I had written against 1.9 failed on 1.8 because 1.8 doesn't allow you to rebind a singleton method from a superclass to the partial mock class object.

@dchelimsky / @alindeman -- can you review what I've done to deal with this shortcoming? I'm worried the warning message may be confusing but I wasn't sure on a better error message. Also, if you guys can think of a way to get it to work on 1.8 and 1.9 that'd be great, too. I've thought of one way but I don't think it's worth doing: we could stash a method object just before a method is mocked or stubbed so that we have that original method object available for use by and_call_original. However, I don't like doing that because it adds extra processing to every single time someone mocks or stubs a method. In many cases there won't be the original method, so a NameError would be raised and I'd have to handle that...and I've always heard that exceptions are known to be slow. I much prefer to grab a method object on demand when and_call_original is used.

@myronmarston myronmarston added a commit that referenced this pull request Oct 26, 2012

@myronmarston myronmarston Merge pull request #23 from myronmarston/can_call_original
Add method to call_original method from a stub
3d278bc

@myronmarston myronmarston merged commit 3d278bc into rspec:master Oct 26, 2012

1 check passed

default The Travis build passed
Details
Contributor

alindeman commented Oct 31, 2012

I'm sorry I wasn't able to review. Is the question you asked above still open?

Owner

myronmarston commented Oct 31, 2012

@alindeman -- Sure. I went ahead and merged but I'm still interested in feedback (particularly about the question I had posted above).

Contributor

alindeman commented Dec 4, 2012

Finally looping back on this: everything looks reasonable to me. Nice work handling all of the edge cases.

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