Rails 4 test failures on ActiveRecord::Associations::CollectionProxy #707

Closed
agrobbin opened this Issue Mar 3, 2013 · 11 comments

Projects

None yet

3 participants

@agrobbin
agrobbin commented Mar 3, 2013

I'm not sure if this is the right place for this issue to be filed, but I thought I would start here.

After upgrading to Rails 4.0.0.beta1, I am experiencing many errors in my test suite related to ActiveRecord::Associations::CollectionProxy::ActiveRecord_Associations_CollectionProxy...

As an example, this test passed on Rails 3.2.12:

let(:institution) { stub_model(Institution) }

describe "#current_user" do
  before do
    controller.stub(:current_institution).and_return(institution)
    controller.stub(:session).and_return({user_id: 'cookie'})
    institution.students.stub(:find_by_cookie).and_return(current_user)
  end

  it "should call #find_by_cookie on students" do
    institution.students.should_receive(:find_by_cookie).with('cookie')
    get :index
  end
end

But, when running on Rails 4.0.0.beta1, this test fails with the following error:

  3) InstitutionsController#current_user should call #find_by_cookie on students
     Failure/Error: institution.students.should_receive(:find_by_cookie).with('cookie')
       (#<ActiveRecord::Associations::CollectionProxy::ActiveRecord_Associations_CollectionProxy_Student:0x007fe078ee2860>).find_by_cookie("cookie")
           expected: 1 time
           received: 0 times
     # ./spec/controllers/institutions_controller_spec.rb:79:in `block (3 levels) in <top (required)>'

If I have missed the issue related to this, or am filing it in the wrong repository, let me know. Thanks!

As a note, the code being tested is below:

def current_user
  return @current_user if defined?(@current_user)
  @current_user = current_institution.students.find_by_cookie(session[:user_id]) if session[:user_id]
end
@myronmarston
Member

I believe that at its core, this is rspec/rspec-mocks#116 dressed up in different clothes. We may want to do something like we did for should and should_not on collection proxies to solve this issue:

https://github.com/rspec/rspec-rails/blob/master/lib/rspec/rails/extensions/active_record/proxy.rb

Long term, the new rspec-mocks syntax we've been discussing in rspec/rspec-mocks#153 will solve this issue for good, as the syntax will no longer be prone to being confounded by proxies.

@alindeman, do you want to add something like the file I linked above?

@agrobbin
agrobbin commented Mar 4, 2013

I was looking at that issue you mentioned @myronmarston , looks like an awesome enhancement!

Let me know if there is anything I should do, posting this in rspec-core, rspec-mocks, or just sitting quietly in the corner :)

@myronmarston
Member

@agrobbin -- you can play around with some configuration like this to see if it solves your problem:

RSpec.configure do |rspec|
  rspec.mock_with :rspec do |mocks|
    mocks.add_stub_and_should_receive_to ActiveRecord::Associations::CollectionProxy
  end
end

Something like that should solve your problem, I think, although it looks like AR is dynamically generating classes there for each association proxy so it may not work with that exact snippet.

@agrobbin
agrobbin commented Mar 4, 2013

Unfortunately not even specifying the full class (ActiveRecord::Associations::CollectionProxy::ActiveRecord_Associations_CollectionProxy_Student) manages to resolve the problem.

@alindeman
Contributor

Darn, these new collection proxies are causing all sorts of trouble. This one is top of my priority to look into.

@alindeman
Contributor

Interesting, I've discovered that calling foo.bars returns a new object each time. This might be the deeper underlying issue because the expectation is set on an object that isn't returned the second time.

As an example:

  it "returns a new object each time" do
    institution = stub_model(Institution)

    expect(5.times.map { institution.students.__id__ }.uniq).to have(1).id
  end

fails with Rails 4.0.0.beta1 with:

 Failure/Error: expect(5.times.map { institution.students.__id__ }.uniq).to have(1).id
       expected 1 id, got 5

So, for instance, this slightly modified spec passes:

  it "passes so long as you make sure to use the same proxy" do
    institution = stub_model(Institution)
    students = institution.students

    students.should_receive(:find_by_cookie).with("cookie")
    students.find_by_cookie("cookie")
  end

This similar spec fails (because each time you call institution.students, you get a new proxy):

  it "passes so long as you make sure to use the same proxy" do
    institution = stub_model(Institution)

    institution.students.should_receive(:find_by_cookie).with("cookie")
    institution.students.find_by_cookie("cookie")
  end

I honestly don't yet know if this is expected behavior in Rails 4. If it is, though, is there really anything rspec/rspec-mocks can or should do?

@alindeman
Contributor

This behavior differs from Rails 3, where the ID is the same:

2.0.0-p0 :019 > p = Post.first
  Post Load (0.2ms)  SELECT "posts".* FROM "posts" LIMIT 1
 => #<Post id: 1, title: "Postzzz", created_at: "2013-03-07 03:00:53", updated_at: "2013-03-07 03:00:53">
2.0.0-p0 :020 > p.comments.__id__
 => 70308212708000
2.0.0-p0 :021 > p.comments.__id__
 => 70308212708000
2.0.0-p0 :022 > p.comments.__id__
 => 70308212708000
@alindeman
Contributor

rails/rails@c86a32d changes the behavior to return a new CollectionProxy each time, rather than the same @proxy object it did in Rails 3.

I've written a comment. We'll see what the response is :)

While this is a bit rough for tests written in the style you show, I don't really think there's much RSpec can do about it. If you have any ideas, drop them here.

@agrobbin

Thanks @alindeman for looking into this. I'll keep an eye on your comment in rails/rails@c86a32d. I agree that the style that I wrote these tests might just need to change if this is how Rails 4 intends to build CollectionProxys. 🐼

@alindeman alindeman referenced this issue in rails/rails Mar 13, 2013
@jonleighton jonleighton CollectionProxy < Relation
This helps bring the interfaces of CollectionProxy and Relation closer
together, and reduces the delegation backflips we need to perform.

For example, first_or_create is defined thus:

class ActiveRecord::Relation
  def first_or_create(...)
    first || create(...)
  end
end

If CollectionProxy < Relation, then post.comments.first_or_create will
hit the association's #create method which will actually add the new record
to the association, just as post.comments.create would.

With the previous delegation, post.comments.first_or_create expands to
post.comments.scoped.first_or_create, where post.comments.scoped has no
knowledge of the association.
c86a32d
@alindeman
Contributor

Should be fixed by rails/rails@133a175 .. let me know if you give it a try!

@alindeman alindeman closed this Mar 16, 2013
@agrobbin

That change in rails/rails fixes it, thanks for following up!

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