DEV: Prefabrication (test optimization) (PR #7414)

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.

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.

:+1: for this.

@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.

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.

One thing that I have been thinking about is whether we have a bad test(s) here. On my local machine, I get a 50 seconds speed up and fabricating an object takes around 0.03 seconds. That means around 1666 object fabrication is being avoided by this PR. Based on what I’m seeing from the code, 1666 objects feel a little off to me. Is it possible for you to do a before and after benchmark for the files affected? Some changes here aren’t gaining from the optimization since there is only one test example in the given context. Having more data here will help us in deciding whether adding a new API is worth it here.

According to test-prof, there are 4389 fewer calls to Fabricate in the tests, resulting in 5568 fewer objects in total.

before:

[TEST PROF INFO] Factories usage

 Total: 22773
 Total top-level: 16217
 Total uniq factories: 119

 total      top-level                            name

  9333           4868                            user
  3278           1731                           topic
  2874           2823                            post
  1459           1459                           admin
   886            774                        category
   542            541                             tag
   528            518                           group
...

after:

[TEST PROF INFO] Factories usage

 Total: 17205
 Total top-level: 11828
 Total uniq factories: 119

 total      top-level                            name

  6885           3267                            user
  2803           1453                           topic
  2618           2573                            post
   692            602                        category
   665            665                           admin
   394            389                           group
   346            345                             tag
...

The changes certainly don’t all have the same impact, it wouldn’t surprise to learn that 20% of the changes result in 80% of the difference. You can see the performance of the affected tests before and after here.

@danielwaterworth Thank you for the stats! I compared some of the results locally on my machine and I’m seeing the same speed ups. Personally, I think this is good to merge since fab! is extensively tested as well.

@SamSaffron and @eviltrout Any thoughts?

OK just to recap:

If we go with let! instead of fab!

Advantages:

  • Less to learn

Disadvantages:

  • Can not do magic super robust handling for non AR objects (so we would have to disallow)
  • Reasoning about ordering is a bit tricky (currently let! happens after before, it would happen after?)

In the Discourse context we can work around the disadvantages, really seeing very few let! out there that would need changing. That said, there is an ongoing risk cause we would be special unicorns and need to reason about let! differently to all other rspec projects.

I really do not like shared_init in many ways this is very focused on prefabrication, not pre-init, and the fact it cleanly resets very much speaks to that.


My proposal for now is create a new gem called rspec-fab with a well documented readme, so we can use it in other rails projects and promote it. Then lets add this dependency to Discourse.

Once rspec-fab is ready lets do this PR and an accompanying post on meta to explain why we arrived at this pattern and so on in the #dev category.

Per @eviltrout make sure there is an “off” switch we can use to debug stuff if needed.

Also going to ask @samphippen here… any thoughts about our current pickle?

If we go with the overriding let! approach, we have to evaluate the block, whatever it contains, before the test runs to find out if it produces an AR object and then it also needs to be evaluated before each test if it’s not an AR object.

I’m happy with your plan - I’ll start working on rspec-fab.

This is ready to take another look at.

Can we keep this change as a separate PR ?

nice, go with FABRICATION_REUSE=0 and just default to 1

Ok

It is in a separate PR, but it’s also required here because things get put into the caches during prefabrication which then need to be invalidated before the tests run.

Just curious, aren’t we going with let! Instead of fab!?

I’ve said that backwards. The problem is that when we jump from test to test in a group, the cache is unaware that the transaction was rolled back. Without prefabrication, the new would evict the old in the setup phase of each test, but now that doesn’t happen.

@SamSaffron just to make sure I understand, is the idea that you want context specific let (or let!) instead of example specific?

@samphippen a lot more subtle than that. The idea here is to get performance of fixtures without the headache of maintaining them.

On first run of the context the rows are created in the database, then on subsequent specs we simply rewind the transaction and then retrieve using the id of the object.

I psuedo code

context 'thing' do
   fab! :post do
      Post.create!(...) # expensive operation calling V8 to cook markdown and so on
      # lets assume it gets post with id #1
   end

    it "is spec 1" do
       # Similar to what `let!` would do on first run
       expect(post.raw).to eq("abc")
    end

   it "is spec 2" do
     # the internals do post = Post.find(1), so all internal state is reset (except for rows in db)
     expect(post.cooked).to eq("abc")
   end
end

@tgxworld no we got to go with fab! mainly cause there is a big conceptual leap we need to be aware of … ordering is slightly diff for example:

context "a" do
   fab! :thing do
       # happens first and only once
       Thing.create!
   end

   before do
     # happens second
   end

   .... 
end

I can not think of particular cases in Discourse where we would still use let! but what we can do in the Discourse context is simply ban it. In the wider ecosystem people may slowly migrate to this pattern.

@SamSaffron @tgxworld, A fairly common problem I’ve been running into is that, often, the tests rely on non-lexical ordering of lets, like this:

describe do
   let(:post) { Fabricate(:post, user: user) }

   context do
      let(:user) { Fabricate(:user) }

      it ...
    end
end

I worked on a tool yesterday to detect instances of this and found 300 tests that rely on this behaviour. I don’t think tests written this way are very clear - you have to jump around the file to figure out what’s happening and it also make prefabrication of those lets impossible. I plan to submit PRs to fix some of these.

Daniel yeah it would be awesome to clean this up!

I mentioned rspec-fab in a ruby chat room and was pointed at:

https://test-prof.evilmartians.io/#/let_it_be

and

https://test-prof.evilmartians.io/#/before_all

Can you test if we can simply alias let_it_be with fab! and just use test-prof instead?