stack level too deep if mocking joins_values attribute accessor (ActiveRecord::Relation) #96

Closed
brueckenschlaeger23 opened this Issue Jan 19, 2012 · 10 comments

Comments

Projects
None yet
4 participants

Too test scopes, I wanted to stub ActiveRecord::Relation joins_values attribute accessor (I was not sure whether it's just an instance variable, so I preferred stubbing):

ActiveRecord::Relation.any_instance.stub(:joins_values).and_return([])

Doing so, leads to a "stack level too deep" exception as soon as I call a scope using joins. Presumably caused by a loop in:

ruby-1.9.2-p290/gems/rspec-mocks-2.8.0/lib/rspec/mocks/any_instance/recorder.rb
where
self.class.__recorder.playback!(self, :#{method_name})
ends up calling
observe!
again.

The error was dificult too localize (- and still not totally sure about it) because first I stubbed "where_values" as well and the error occured in OTHER test cases than the test case where I defined the stub. - Presumably the error came during cleanup?

It seems like an bug to me. If not, this behavior (don't stub attr_accessors) should be specified.

Owner

dchelimsky commented Jan 19, 2012

attr_accessors are just methods in the end, so stubbing them is no problem.

It does seem like a bug to me, but not RSpec's. Take a look at https://github.com/rails/rails/blob/422958f17cbe1b6009313b7acd01cc11aacec3d7/activerecord/lib/active_record/relation.rb#L482-484 and note that the @joins_values instance variable is being referenced directly, and will likely not have the value that you're example is stubbing on the accessor. I'm not sure that this is the issue, but this is the reason that stubbing internals of 3rd party libs is generally a bad idea. You should stick to stubbing methods that you're in control of as much as possible.

I'd recommend spec'ing scopes by creating real DB records and making sure you get the right set of records back.

As for discovering the specific source of this problem, I can help debug it but I need access to your app or a small sample app that I can clone, bundle, run and see the error.

I'm closing this as I really don't think it's an RSpec bug, but if debugging it proves otherwise I'll reopen it.

dchelimsky closed this Jan 19, 2012

I replaces all the rails instance_attribute accesses like @where_values, @joins_values etc. by where_values in different files. It's partly true, the "stack level too deep" error got solved for where_values, but not for joins_values. ([EDIT]: @joins_values is accessed by squeel, too. Removing @ leads to free scope mocking with squeel)

Just one thing again, look at
./gems/ruby-1.9.2-p290/gems/rspec-mocks-2.8.0/lib/rspec/mocks/any_instance/recorder.rb#observe! - method

Wouldn't it be cool if __recorder.playback!(self, :#{method_name}) just would not call observe!(method_name) again for the same method and same (singleton-) object? Especially, I do not understand why it happens.

Even if it's a rails bug, the returned scoped values for @joins_values are like expected and the stack-level too deep is raised by rspec_mocks. Maybe you have time to have a look at recorder.rb again?

I'll provide a test app as soon as I have time.

BTW, we cannot "making sure you get the right set of records back" because in the test case, other parts (out of select, joins, etc.) of the scope are transparent. There is no proper solution apart from mocking to test only one

@dchelimsky dchelimsky added a commit that referenced this issue Jan 20, 2012

@dchelimsky dchelimsky don't re-observe methods that are already being observed
Experimental fix for #96.
322059f

dchelimsky reopened this Jan 20, 2012

Owner

dchelimsky commented Jan 20, 2012

@brueckenschlaeger23 - I modified observe! per what I think is your suggestion and all the specs passed. Would you do me a favor and point your Gemfile at the issue-96 branch and let me know if it has any impact on this issue for you?

gem "rspec-mocks", :git => "https://github.com/rspec/rspec-mocks.git", :branch => "issue-96"

re: your inability to test in a certain way, there is an old TDD saying that you should listen to your tests; that if something is hard to test it probably means you have a coupling problem in your design. I don't know what your design looks like, but it's probably something to consider.

Thanks a lot! Indeed, the changes fix the stack-level too deep issue. Hopefully you can merge it back soon!

Thanks again for your advise, but really, as I told, I have an environment to generate tests and on a certain state, other parameters of the scope are simply hidden/transparent and there is nothing I can do about it.
Besides, it's really cool, clean and intuitive to test scopes by mocking other params accordingly. Thanks again!

A, BTW sorry for answering so late, I was not around for a month.

Contributor

alindeman commented Jun 10, 2012

Any news on this one? @dchelimsky, can issue-96 be merged?

@dchelimsky dchelimsky added a commit that referenced this issue Jun 10, 2012

@dchelimsky dchelimsky don't re-observe methods that are already being observed
Experimental fix for #96.
fe27fe2
Owner

dchelimsky commented Jun 10, 2012

@alindeman as soon as one of us adds a spec :)

Owner

dchelimsky commented Jun 10, 2012

I just rebased and force pushed that branch, BTW.

alindeman was assigned Sep 9, 2012

Owner

myronmarston commented Dec 24, 2012

@alindeman -- are you still able to tackle this?

I was going to take a look at adding the missing spec and merge the fix; however, I don't understand what the conditions are that trigger this bug. Would be nice if someone could explain how this bug can be manifested outside rails so one of us can write a spec for it.

Contributor

alindeman commented Dec 27, 2012

I've lost my context, but can definitely take a look again.

Contributor

alindeman commented Dec 28, 2012

I'm unable to reproduce this exception when stubbing joins_values on rspec-mocks 2.8.0 or 2.12.0. However, it smells so much like #167 (which has been fixed) that I feel comfortable closing it.

However, @brueckenschlaeger23, if you're able to reproduce this on 2.12.x and can write us a small--but self-contained--test case, please ping me to reopen. Thanks!

alindeman closed this Dec 28, 2012

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