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

DEV: Prefabrication (test optimization) #7414

Merged
merged 4 commits into from May 7, 2019

Conversation

10 participants
@danielwaterworth
Copy link
Member

commented Apr 22, 2019

Whereas:

let!(:user) { Fabricate(:user) }

Creates a user object in the database for each test.

prefabricate(:user) { Fabricate(:user) }

Creates a user object in the database for each group of tests. These
objects get cleaned up by virtue of being created in a transaction which
is rolled back when the group completes.

@discoursebot

This comment has been minimized.

Copy link

commented Apr 22, 2019

You've signed the CLA, danielwaterworth. Thank you! This pull request is ready for review.

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch from eb7d696 to bd08b09 Apr 22, 2019

@danielwaterworth danielwaterworth marked this pull request as ready for review Apr 22, 2019

@discoursedangerbot

This comment has been minimized.

Copy link

commented Apr 22, 2019

1 Warning
⚠️ This pull request is big! We prefer smaller PRs whenever possible, as they are easier to review. Can this be split into a few smaller PRs?

Generated by 🚫 Danger

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

what kind of performance difference does this make to the test suite?

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

This PR makes the tests ~10% faster, but there are still many unexploited opportunities to apply this.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

The performance win is something that I love, but I worry that the new syntax makes it harder for people to author tests, they always need to think, should I be using prefab vs let?

What I wonder is if somehow we can:

  1. Clear a bunch of rows in the test db.
  2. Prefabricated a pile of objects and rows in the test DB and hold refs
  3. Start the test suite (leaning on (1)) so the db is no longer "blank" when all the tests start.

Then simply amend the behaviour of Fabricate to make use of (2), so the end result is the individual tests don't need to be aware of this.

The trouble with (1) and (2) is that it would add cost to every time we run a single test in the suite. That said we could have logic that makes (1) and (2) only run on full test suite runs.

I am not sure this is all pretty speculative but I worry about adding all this new syntax in the PR and decision points for devs.

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

My first attempt to solve this problem was similar to what you suggest. It would build a set of objects before any tests were run. I abandoned this effort because, unfortunately, a significant number of the tests assume they are starting with a blank slate.

As far as deciding between prefabricate vs let (vs let!), this is how I think about it:

If the choice was just between let vs let! and performance wasn't a concern, I would always choose let!. It's easier to predict what a test using let! will do since you don't have to figure out what order the let bodies will execute in or if they will execute at all and this matters when there are side effects.

In this light, let is a cheaper let!. It's almost observationally the same, except in the presence of side effects or divergence.

However, prefabricate is also just a cheaper let! and is also almost observationally equivalent. Moreover, prefabricate is cheaper or equal to let as long as the thing in question is actually used.

So, my answer to "which should I use?" is "default to prefabricate for active record objects".

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

I follow ... maybe the name fab! here will alleviate some of the concerns. Cause it is both clear and short.

prefabricate is both a mouthful to type and is kind of surprising cause we are used to let only sometimes is happening.

@eviltrout / @tgxworld what are your feelings here on introducing a new fab! method that allows reuse of objects between tests in a spec file?

Also to clarify is fidelity of "group" here? Is this per context?

eg: in this example then Fabricate(:user) will be called twice?:

context "test1" do
  fab!(:user) { Fabricate(:user ) } 
   it "something 1" do
     ....
   end
    it "something 2" do
     ....
   end
end
context "test" do
   fab!(:user) { Fabricate(:user ) } 
    it "something 2" do
     ....
   end
end
@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

Changing prefabricate to fab! is a great suggestion.

Your intuition is correct, the user is created twice and it won't exist in the database when its not in scope.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Also how is state leak handled? I am kind of OK to have rules about immutability in some cases, but want to know what the base rule is here?

context "test" do
   fab!(:user) { Fabricate(:user) }

    it "test 1" do
       user.name = "bob17"
       DB.exec("update users set name = ? where id = ?", "testing", user.id)
       user.save!
    end

    it "test 2" do
         puts user.name
         puts user.username
    end
end

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

You are able to mutate the objects in the database, you can even delete them. There's a transaction around each test and there's a transaction around each group with prefabricated objects (rails fakes nested transactions with savepoints).

So, you are completely able to mutate a prefabricated object with arbitrary SQL provided you do so on the default DB connection. These objects aren't visible on any other connection because the transaction never commits. Even without this PR, this rule already applies to objects created during tests.

As far as state that doesn't get persisted is concerned, the object itself is created afresh each test from its id. So, it's also fine to mutate each object's unpersisted state, as long as you don't expect unpersisted state that you create in the fab! block to be present in the test itself.

prefabricated_classes = @prefabricated_classes
prefabricated_ids = @prefabricated_ids

define_method(name) do

This comment has been minimized.

Copy link
@tgxworld

tgxworld Apr 23, 2019

Member

Wouldn't this leak the methods into other examples?

This comment has been minimized.

Copy link
@danielwaterworth

danielwaterworth Apr 23, 2019

Author Member

I don't believe so. RSpec creates a class for each context and define_method puts the method on the class, not the module, as illustrated here:

module Test
  def foo
    define_method(:bar) do
      :bar
    end
  end
end

class Foo
  extend Test

  foo
end

class Bar
  extend Test
end

p Foo.new.respond_to?(:bar) # => true
p Bar.new.respond_to?(:bar) # => false

This comment has been minimized.

Copy link
@danielwaterworth

danielwaterworth Apr 23, 2019

Author Member

I've added specs for fab! to demonstrate that it does what you'd expect w.r.t scoping.

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch from 53a7889 to 9b936f1 Apr 23, 2019

@tgxworld

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

@danielwaterworth Do you have the actual numbers across multiple runs? I'm curious if it is a consistent 10% gain here. I think the usage is quite interesting here even though it feels like a per context fixture. How about overriding let! instead? That maintains the same API but comes with the per context cache introduced here.

Also the code looks fine to me but can you try running the whole test suite 10 times to ensure there aren't any failures as a result of the change made in this PR? I see that Travis has been failing a couple of times here so that makes me worry.

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch 3 times, most recently from 7243a50 to 0373edf Apr 23, 2019

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented Apr 23, 2019

@tgxworld, You're right, with the tests taking so long to run, it is difficult to get statistical accuracy. I've just done another run after rebasing, it came out at 13m29s vs 14m27s for master - which is only a 7% improvement. It's much easier to become convinced of the efficacy of these changes by running individual spec files before and after instead.

I've also noticed the failures. I'm looking into that now. There are also intermittent failures on master that I'm looking into as well.

One problem with overriding let! is that fab! only works for active record objects and let! can be used for any ruby object.

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch from 47dd10b to 650f909 Apr 23, 2019

@tgxworld

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Nice I ran this locally and saw a speed up of about 50 seconds from 9mins 32 seconds.

@eviltrout

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

My biggest concern here is the potential for this to introduce leaky state / bugs that might introduce heisentests in the future. The speed gain is nice, but there is a risk of more hard to catch bugs as a result.

Maybe we could introduce an option to disable the behavior for easy diagnosis?

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch from 6ae31c1 to 0a252fd Apr 23, 2019

@@ -11,6 +11,10 @@
RateLimiter.enable
end

after do
RateLimiter.disable

This comment has been minimized.

Copy link
@tgxworld

tgxworld Apr 23, 2019

Member

Will it be better to move the disabling of rate limiting into an after(:each) instead?

RateLimiter.disable

This comment has been minimized.

Copy link
@danielwaterworth

danielwaterworth Apr 24, 2019

Author Member

@tgxworld, I completely agree. The next commits actually make this unnecessary so it's unlikely that this one will make it into the final PR.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

I am with @eviltrout in that we should have a switch to disable this optimisation just-in-case maybe just an env var?

One problem with overriding let! is that fab! only works for active record objects and let! can be used for any ruby object.

I wonder though if that is an unsolvable problem? you could look at the object you get back from let! and if ActiveRecord::Base === o then go down one path else go down another.

I am kind of leaning towards just "turbo boosting" let!, for a few reasons.

  1. This general pattern will be easier to upstream to rspec as an option
  2. Less stuff to teach devs

Overall, this looks like a very safe change to me, only real risk here heisentest wise are going to be static methods that leaks objects, but I feel the risk is tiny and it already exists in all tests anyway. The transaction savepoint trick is saving us from enormous amounts of danger here.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Also, once we reach our final verdict here, I would like a post on meta in the #dev category explaining how this optimisation works and so on, this is something I would like to share with the greater Ruby community.

@tgxworld

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

I wonder though if that is an unsolvable problem? you could look at the object you get back from let! and if ActiveRecord::Base === o then go down one path else go down another.

👍 for this.

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch 2 times, most recently from 9090990 to 73ede1b Apr 24, 2019

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2019

I understand that there's a tension here.

On the one hand, it's in nobody's interests to put hurdles in front of developers and, since this deviates from rails norms, this is a hurdle.

On the other hand, the performance gains are significant which will improve the development experience.

As much as I'd like a have-your-cake-and-eat-it scenario, I don't think calling fab! let! and making it gracefully handle non active record objects is it. I think there are enough differences between fab! and let! that they deserve to be referred to differently. How should these cases be handled, for example?

let!(:users) { [Fabricate(:user, name: 'foo'), Fabricate(:user, name: 'bar')] }
let!(:user) { Fabricate(:user) }
before do
  # Important step to perform before let! block
end
let!(:user) { Fabricate.build(:user) }
# acting_user isn't persisted
let!(:post) { Fabricate(:post, acting_user: Fabricate(:user)) } 

There are other ways to differentiate between the two cases than we have discussed so far. How would you feel about something like this?

context "in an alternative universe" do
  shared_init do
    # fab! style let!
    let!(:user) { Fabricate(:user) }
  end
  # regular let!
  let!(:important_number) { 1 } 
end

To me it says that I can continue to use my understanding of let!, but it also hints that something a little different is happening.

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch 5 times, most recently from f58a456 to 8de12fb May 1, 2019

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@samphippen, I think you're right - having a let equivalent at the group level would be helpful. There are many places in the tests where strings or collections of arguments are put in a let block and at present, fab! can't refer to those things.

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch 2 times, most recently from 0d2a0b8 to d5b118a May 2, 2019

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented May 2, 2019

@SamSaffron, could you take another look at this? If you're happy with it, I'd like to see it merged and the floodgates can be opened for contingent PRs.

Gemfile Outdated
@@ -123,7 +123,7 @@ group :test do
gem 'fakeweb', '~> 1.3.0', require: false
gem 'minitest', require: false
gem 'simplecov', require: false
gem "test-prof"
gem 'test-prof', git: 'https://github.com/danielwaterworth/test-prof.git'

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron May 3, 2019

Member

This is a bit of a blocker for me, can you push a temporary gem then until all the PRs are merged? or just carry a monkey patch locally till a new test-prof is out?

In the past we have had issues with git dependencies in our Gemfile and even though this is only for test I am uneasy at the moment the only exceptions we allow are imports and rails master testing.

@@ -121,14 +122,14 @@ class Category < ActiveRecord::Base
# Allows us to skip creating the category definition topic in tests.
attr_accessor :skip_category_definition

@topic_id_cache = DistributedCache.new('category_topic_ids')
has_distributed_cache :topic_id_cache, 'category_topic_ids'

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron May 3, 2019

Member

I think just name this distributed_cache no need for the has_ cause it can be a bit confusing, people may think it is active recordy (has_one , has_many)

def has_distributed_cache(name, key, **opts)
define_singleton_method(name) do
HasDistributedCache.dirty.add(object_id)
HasDistributedCache.caches[object_id] ||= {}

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron May 3, 2019

Member

if you simply set HasDistributedCache.caches to {} at the end of every test run no need to track dirty ? no?

This comment has been minimized.

Copy link
@danielwaterworth

danielwaterworth May 3, 2019

Author Member

@SamSaffron, This comment was extremely helpful.

I had the same thought yesterday, but I could see a clear performance regression when I did that. It was looking like master vs distributed caches with dirty tracking were roughly equivalent. I reasoned that there must be an important difference around cache initialization vs clearing.

When I revisited it this morning, with a fresh brain, that explanation didn't hold water for me. I've rerun the tests with and without dirty tracking a few times and the difference isn't so pronounced as I thought it was. I looked into this further and now I suspect the extra ~40 seconds isn't down to redis traffic or cache initialization at all. It's just that the tests use cached values from prior tests.

Here's the data. This is cache hits by cache where the value was written in a different test:

["icon_manager", 10854]
["theme", 6829]
["am_serializer_fragment_cache", 4840]
["banner_json", 1299]
["scheme_hex_for_name", 1263]
["discourse_stylesheet", 311]
["category_topic_ids", 148]
["svg_sprite", 66]
["csp_extensions", 12]
["developer_ids", 4]
["last_read_only", 0]
["category_url", 0]

When you give each test a clear cache, it rebuilds it by doing queries against the DB. It seems obvious in retrospect, but these things often do.

My plan is to prime these caches explicitly before running the test suite and preventing changes unless opted into.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented May 3, 2019

Sure, last minor round of feedback and I will merge this in first thing on Monday (Australia time)

danielwaterworth added some commits Apr 23, 2019

Introduced fab!, a helper that creates database state for a group
It's almost identical to let_it_be, except:

 1. It creates a new object for each test by default,
 2. You can disable it using PREFABRICATION=0
Undid theme prefabrication
Themes have complex interactions with caches that need to be handled
before this can be undone.

@danielwaterworth danielwaterworth force-pushed the danielwaterworth:prefabrication branch from d5b118a to 59bb8ad May 5, 2019

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@SamSaffron, I should said so earlier, but this is ready to be looked at again.

@SamSaffron

This comment has been minimized.

Copy link
Member

commented May 6, 2019

@SamSaffron

This comment has been minimized.

Copy link
Member

commented May 7, 2019

🎊 merging this in (once I resolve the conflict ... my current test time is 7:10 ... lets see what this does

Thanks heaps for this work Daniel!

@SamSaffron SamSaffron merged commit e219588 into discourse:master May 7, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@SamSaffron

This comment has been minimized.

Copy link
Member

commented May 7, 2019

OMG

6:06s down from 7:10s for 11146 specs this is not too shabby at all, awesome work!

@danielwaterworth danielwaterworth deleted the danielwaterworth:prefabrication branch May 7, 2019

@danielwaterworth

This comment has been minimized.

Copy link
Member Author

commented May 7, 2019

Thank you! It's great to see this merged 😄

@palkan palkan referenced this pull request May 7, 2019

Open

Success stories #73

@pcreux

This comment has been minimized.

Copy link

commented May 7, 2019

I wish I've done a better job spreading the word about https://github.com/pcreux/rspec-set since 2010. 😅 It's the exact same idea. Just called set instead of fab!.

Edit: Back in the days, contexts were not wrapped in SQL transactions themselves, so objects created using set were leaking across tests.

@palkan

This comment has been minimized.

Copy link

commented May 7, 2019

@pcreux Yeah, someone told me about rspec-set when I was talking about test-prof. If I knew about it before, I'd prefer to contribute to it or integrate into test-prof)

The idea is very similar though with one significant difference:

Back in the days, contexts were not wrapped in SQL transactions themselves

That remains the same: fab! (or more precisely, before_all) takes care of it. Though this feature will probably find its way to the rspec-rails upstream someday.

@supairish

This comment has been minimized.

Copy link

commented May 8, 2019

It appears Instructure also created a gem to tackle this problem https://github.com/instructure/once-ler I'm just linking for reference. Seems a lot of people have tried solving this before 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.