and_call_original does not always properly restore the method

[myronmarston]

I discovered this while working on #612, when I migrated some specs from using unstub to using and_call_original. I marked the ones that failed as pending:

  unstubbing with `and_call_original` restores the correct implementations when stubbed and unstubbed on a parent and child class
    # not working for `and_call_original` yet, but works with `unstub`
    # ./spec/rspec/mocks/stub_implementation_spec.rb:57
  #any_instance unstubbing using `and_call_original` removes stubs even if they have already been invoked
    # not working for `and_call_original` yet, but works with `unstub`
    # ./spec/rspec/mocks/any_instance_spec.rb:296
  #any_instance unstubbing using `and_call_original` removes stubs from sub class after invokation when super class was originally stubbed
    # not working for `and_call_original` yet, but works with `unstub`
    # ./spec/rspec/mocks/any_instance_spec.rb:306

[yujinakayama]

I've found the same issue while implementing the conversion of obj.unstub(:message) to allow(obj).to receive(:message).and_call_original in Transpec.

• https://travis-ci.org/yujinakayama/transpec/jobs/19816822#L1098
• https://github.com/yujinakayama/guard/blob/transpec-test-rspec-2-99/spec/guard/ui_spec.rb#L10-L14

You can reproduce this with the following steps:

$ git clone https://github.com/yujinakayama/transpec.git
$ cd transpec 
$ git checkout support-conversion-of-unstub
$ bundle install
$ bundle exec rake test:guard

The guard project can be found in tmp/tests/guard.

[myronmarston]

Thanks, @yujinakayama. I've updated the labels on this from "not a release blocker" to "release blocker" since I think it's important that we get this to work so transpec can convert.

[xaviershay]

I am looking at this.

[xaviershay]

Ok there are some serious dragons here.

First Dragon

Those guard specs are actually relying on caller to figure out what the calling plugin is: https://github.com/yujinakayama/guard/blob/transpec-test-rspec-2-99/lib/guard/ui.rb#L178

When using unstub, caller is shallow:

tests/guard/lib/guard/ui.rb:164:in `filter'
tests/guard/lib/guard/ui.rb:64:in `warning'
tests/guard/spec/guard/ui_spec.rb:100:in `block (3 levels) in <top (required)>'

When using and_call_original, rspec appears in the backtrace:

tests/guard/lib/guard/ui.rb:164:in `filter'
tests/guard/lib/guard/ui.rb:64:in `warning'
rspec-mocks-3.0.0.beta1/lib/rspec/mocks/message_expectation.rb:620:in `call'
rspec-mocks-3.0.0.beta1/lib/rspec/mocks/message_expectation.rb:620:in `call'
rspec-mocks-3.0.0.beta1/lib/rspec/mocks/message_expectation.rb:243:in `invoke'
rspec-mocks-3.0.0.beta1/lib/rspec/mocks/proxy.rb:164:in `message_received'
rspec-mocks-3.0.0.beta1/lib/rspec/mocks/method_double.rb:82:in `proxy_method_invoked'
rspec-mocks-3.0.0.beta1/lib/rspec/mocks/method_double.rb:69:in `block (2 levels) in define_proxy_method'
tests/guard/spec/guard/ui_spec.rb:91:in `block (3 levels) in <top (required)>'

So the original method is being called fine, but since the backtrace is different it is causing spec failures.

Second Dragon

Replacing methods in a class hierarchy acts really weird.

class A
  M = A.method(:new)

  def self.new
    M.call
  end
end
class B < A; end

puts B.new # #<A:0x007f9bd408d438>

@myronmarston what was the initial motivation for replacing unstub? Based on the above I feel like perhaps it isn't actually semantically equivalent to and_call_original. In my head they do mean different things, even though they would be effectively equivalent in most places.

To fix at least the first issue above, and_call_original would actually have to delegate to unstub (or patch caller ). That doesn't seem like something we want to do - we'd only be able to get away with it if there were no other expectations set, but then we'd be subtlety changing semantics for very similar APIs.

My current recommendation is that transpec not perform an automatic conversion from unstub to and_call_original. We may still want to try and fix the second issue, I'm just not sure how you would do it.

[myronmarston]

@myronmarston what was the initial motivation for replacing unstub?

There was never an intentional plan to replace unstub. In the original issue about the new expect/allow syntax (#153) we discussed unstub and tried to figure out what the new syntax for that feature would be. We couldn't come up with anything anyone was happy with, and the consensus was that it was a rarely used feature that no one could really articulate a use case for.

Later on, in #486, I realized that and_call_original could be used to replace unstub (or so I thought). In my mind they have the same semantics: and_call_original is meant to make the object respond as it originally did and unstub is meant to likewise make the object respond as it originally did. The one difference I can think of is that and_call_original was originally intended for message expectations (expect(obj).to receive(:foo).and_call_original) where it still needs to redefine the method and observe it being called -- whereas unstub has no such requirement. and_call_original with allow should theoretically not need to observe the method either, unless it's being used in combination with expect. Actually, now that I think about it -- even if it's not being used with expect we need to observe the method so that have_received can work with it.

In my head they do mean different things, even though they would be effectively equivalent in most places.

What is the different meaning in your head?

Anyhow...this is all say I'm not sure what the right thing to do here is. I agree that the guard caller thing is a special case that I don't think we're going to be able to fix given that we still have to observe the method being called and therefore will affect the backtrace.

[myronmarston]

BTW, @xaviershay, did you take a look at the pending examples for this?

Those are cases where and_call_original isn't working as expected, regardless of issues around guard's usage.

[xaviershay]

What is the different meaning in your head?

and_call_original says we're still observing it, unstub says "we're out of here".

pending examples

the first one I looked and at don't know how to solve (that's the second dragon). The any_instance ones I haven't looked at.

[fables-tales]

I took a look at this and started building a model of what's going on in my head. It's something to do with the original method being captured incorrectly on subclasses when both classes are already stubbed.

[myronmarston]

So I've started digging into this and now I understand what's going on.

With normal (e.g. not class) objects, when we define a singleton method on that object, that object is the only object affected. The only previously stubs that could affect that object are stubs on the object itself that the method_double will be aware of.

Classes are different: when you define a singleton method on it, it affects subclasses, too, due to how ruby's method lookup works with classes.

So here's what's going on:

      it "restores the correct implementations when stubbed and unstubbed on a parent and child class" do
        parent = stub_const("Parent", Class.new)
        child  = stub_const("Child", Class.new(parent))

        allow(parent).to receive(:new)
        allow(child).to receive(:new)
        allow(parent).to receive(:new).and_call_original
        allow(child).to receive(:new).and_call_original

        expect(parent.new).to be_an_instance_of parent
        expect(child.new).to be_an_instance_of child
      end

The parent's and_call_original works because there isn't an upstream superclass that's had the method stubbed. The child's and_call_original does not work because when it captures the original_method the logic isn't aware that the existing behavior of child.new at that point in time has already been changed by the parent method being stubbed, so it gets the wrong Method object (e.g. the parent's new) and thereby works wrongly.

I'm working on a fix. It's tricky but not insurmountable.

[JonRowe]

Sounds similar to the prepend issue...

[myronmarston]

I'm going to take a look at the any_instance cases next.

[myronmarston]

So it turns out that the failing and_call_original / any_instance specs have nothing to do with and_call_original. It's a more general problem than that. The problem is that once an instance has been stubbed by the any instance recorder playing back onto its proxy, future any_instance changes to the method no longer affect the instance. For example, this fails:

          it "can change how instances responds in the middle of an example" do
            instance = klass.new

            allow_any_instance_of(klass).to receive(:foo).and_return(1)
            expect(instance.foo).to eq(1)
            allow_any_instance_of(klass).to receive(:foo).and_return(2)
            expect(instance.foo).to eq(2)
          end

fails with:

     Failure/Error: expect(instance.foo).to eq(2)

       expected: 2
            got: 1

       (compared using ==)

I'm working on a fix for this. @yujinakayama, I think it's safe to make transpec convert to and_call_original again now that I fixed the other issue. As @xaviershay pointed out it's not exactly equivalent to unstub (since we still observe the method but unstub doesn't) but in practice I think the only place where this matters is when the method has call-stack dependent logic, but that's rare enough that I think it's OK to convert.

[JonRowe]

As @xaviershay pointed out it's not exactly equivalent to unstub (since we still observe the method but unstub doesn't) but in practice I think the only place where this matters is when the method has call-stack dependent logic, but that's rare enough that I think it's OK to convert.

👍

[yujinakayama]

@myronmarston Thanks! 👍

[myronmarston]

Closing in favor of #651.