rspec / rspec-mocks

RSpec's 'test double' framework, with support for stubbing and mocking
https://rspec.info
MIT License
1.16k stars 356 forks source link

SystemStackError when decoration is involved #1016

Open agrimm opened 8 years ago

agrimm commented 8 years ago

Given the following the RSpec,

describe UserDecorator do
  let(:user) { User.new }
  let(:decorated_user) { user.decorate }

  describe '#greeting' do
    # Failing spec
    it 'uses DecoratedUser#name' do
      expect(decorated_user).to receive(:name).and_call_original
      decorated_user.greeting
    end

    # Successful version
    it 'uses User#name' do
      expect(user).to receive(:name).and_call_original
      decorated_user.greeting
    end

    # Also successful, because it's now somehow properly set up
    it 'again uses DecoratedUser#name' do
      expect(decorated_user).to receive(:name).and_call_original
      decorated_user.greeting
    end
  end
end

I get the following results

Andrew-Grimms-MacBook-Pro:rspec_stack_replication agrimm$ bundle exec rspec
F..

Failures:

  1) UserDecorator#greeting uses DecoratedUser#name
     Failure/Error: decorated_user.greeting
     SystemStackError:
       stack level too deep
     # ./app/decorators/user_decorator.rb:5:in `greeting'
     # ./spec/decorators/user_decorator_spec.rb:11:in `block (3 levels) in <top (required)>'

Finished in 2.98 seconds (files took 1.27 seconds to load)
3 examples, 1 failure

Failed examples:

rspec ./spec/decorators/user_decorator_spec.rb:9 # UserDecorator#greeting uses DecoratedUser#name

It seems that making the decorated object the target of the spec avoided the SystemStackError.

Also, once the code is exercised a bit, it seems to work ok. The third test is the same as the first, but it passes (the specs are order dependent in this case).

I've created an example app to replicate this issue. https://github.com/agrimm/rpsec_stack_replication

This involved RSpec 3.3.2, and Draper 2.1.0.

I tried searching for bugs involving SystemStackError, but they seemed to involve any_instance_of, unlike this bug.

Apologies if I'm reporting this on the wrong sub-gem - I assume it's a mocking issue.

myronmarston commented 8 years ago

Thanks for reporting this! This is definitely a bug but it's one that's not easily solved. I boiled it down into a failing spec in c7efa414e88c840fe4824cb011948cc1b3296d69 (just pushed into the expose-issue-1016 branch). Here's what's going on, using your reproduction repo as an example:

  1. On line 10 you do expect(decorated_user).to receive(:name).and_call_original. This causes rspec-mocks to get the original method so that it can later call it.
  2. However, draper does not define the delegated methods on the decorator ahead of time -- instead it uses method_missing to lazily define them on first use. That means that when that line runs, there is no original method for rspec-mocks to get. rspec-mocks handles method_missing, though; in this case it generates a proc in place of the original method that, when called, will trigger method_missing on the object, replicating the original behavior the object would have had (in theory, at least -- it falls over in this case).
  3. Because you've mocked the method, rspec-mocks defines a singleton method on the object that it uses to track the fact the method was called and to delegate to the original method object it stored earlier.
  4. On line 11 you call greeting which internally calls name. This is the method that rspec-mocks has defined on your object.
  5. The implementation calls the proc we generated earlier, which in turn calls method_missing on the object.
  6. Draper's method_missing implementation defines the method on the decorator class using delegate from active support.
  7. Then draper turns around and attempts to invoke the method it just defined.
  8. Because rspec-mocks had defined a singleton method on the object, it takes precedence over the instance method draper just defined, and rspec-mocks method is invoked.
  9. rspec-mocks' method again calls the proc that delegates to method_missing and the process repeats itself.

The basic problem is that and_call_original, as currently implemented, is not able to work with objects that implement the method lazily in method_missing and then call it. I've tried a few things but haven't been able to come up with a fix yet.

thanhlelgg commented 6 years ago

Anyone is working on this one?

myronmarston commented 6 years ago

Not that I know of.

Sent from my iPhone

On Jun 8, 2018, at 2:39 AM, Thanh Le notifications@github.com wrote:

Anyone is working on this one?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

agrimm commented 6 years ago

@thanhlelgg I'm not working on it.