Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

speed up fixtures by not loading all their classes #17014

Merged
merged 1 commit into from Oct 10, 2014

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Sep 22, 2014

this took our time for a test with tixtures :all from 28s to 20s

most of the time, only the tables are needed and fixtures :all is just a developer saying "I don't know",
so let's save the complexity and overhead!

people that rely on that can do their own requires or use eager loading in test.rb

@rafaelfranca @tenderlove

@rafaelfranca
Copy link
Member

Does it require any adicional step from people that already use fixture :all?

@grosser
Copy link
Contributor Author

grosser commented Sep 22, 2014

if autoload works: no
if they disabled autoload: maybe ...

@rafaelfranca
Copy link
Member

I don't like this:

people that rely on that can do their own requires or use eager loading in test.rb

People will not know why their tests are failing after this change.

@rafaelfranca
Copy link
Member

Also if we are trading predictability for speed I prefer to keep the predictability.

cc @fxn @jeremy

@grosser
Copy link
Contributor Author

grosser commented Sep 22, 2014

people that disabled autoload that is ... so not sure how many % that is
also if you don't use autoload, chances are you require all your things up
front anyway ...
so 1-5% of 1-5% of users will get a 'Undefined class User' when updating
and then have to add a pretty straight forward require 'user'

On Mon, Sep 22, 2014 at 1:31 PM, Rafael Mendonça França <
notifications@github.com> wrote:

I don't like this:

people that rely on that can do their own requires or use eager loading in
test.rb

People will not know why their tests are failing after this change.


Reply to this email directly or view it on GitHub
#17014 (comment).

@@ -870,34 +870,9 @@ def fixtures(*fixture_set_names)
end

self.fixture_table_names |= fixture_set_names
require_fixture_classes(fixture_set_names, self.config)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 We already look up model classes by constantizing anyway.

As far as I can tell, requiring them here predates that, so we're doing duplicate work now.

@grosser
Copy link
Contributor Author

grosser commented Sep 22, 2014

FYI monkeypatch in case you want to try it out:

# do not load every class when using fixtures :all -> speedup
ActiveRecord::TestFixtures::ClassMethods.class_eval do
 def require_fixture_classes(*args)
 end
end

@tenderlove
Copy link
Member

@rafaelfranca as @jeremy points out, the autoload will get fired anyhow since we do a constant lookup, so this code just seems like extra work.

@grosser
Copy link
Contributor Author

grosser commented Oct 7, 2014

fixed the tests, good to merge ?

@grosser
Copy link
Contributor Author

grosser commented Oct 10, 2014

@tenderlove / @jeremy merge ?

tenderlove added a commit that referenced this pull request Oct 10, 2014
speed up fixtures by not loading all their classes
@tenderlove tenderlove merged commit 5127857 into rails:master Oct 10, 2014
@tenderlove
Copy link
Member

@grosser THANKS! <3<3<3<3 :-D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants