palkan / n_plus_one_control

RSpec and Minitest matchers to prevent N+1 queries problem
MIT License
553 stars 20 forks source link

Allow defining `let` in populate #48

Closed andresespinosapc closed 2 years ago

andresespinosapc commented 3 years ago

Is your feature request related to a problem? Please describe.

I want to test a show endpoint instead of index, so the request is specific to a record and I define it on a let variable.

Describe the solution you'd like

To allow defining a different let variable each time the populate block is called.

Describe alternatives you've considered

The only way I found is to directly use Record.last when making the request, but it doesn't work if I use it from a let variable. This is not very clean nor DRY.

palkan commented 3 years ago

Could you please provide an example test code? That would help us to come out with a proper API.

gagalago commented 2 years ago

for now, we have to do (see Page.first in the expect):

populate { |number| create_list(:pages_work, number, page: create(:page)) }
it "doesn't trigger n+1 query" do
  expect { get "/pages/#{Page.first.id}/pages_works" }.to perform_constant_number_of_queries
end

it can be great if we can do

let(:page) { create(:page) }
populate { |number| create_list(:pages_work, number, page: page) }
it "doesn't trigger n+1 query" do
  expect { get "/pages/#{page.id}/pages_works" }.to perform_constant_number_of_queries
end

or at least

populate do |number| 
  let(:page) { create(:page) }
  create_list(:pages_work, number, page: page)
end
it "doesn't trigger n+1 query" do
  expect { get "/pages/#{page.id}/pages_works" }.to perform_constant_number_of_queries
end
gagalago commented 2 years ago

by writing the previous comment, I just realize what is the solution:

let(:page) { create(:page) }
before { page } # force creation at this point
populate { |number| create_list(:pages_work, number, page: page) }
it "doesn't trigger n+1 query" do
  expect { get "/pages/#{page.id}/pages_works" }.to perform_constant_number_of_queries
end

the trick is that you have to ensure that data that need to be the same in different scale are persisted before the call to populate, otherwise due to the lazy nature of let that will be done in the populate that is reverted (thanks to transaction) between each scale factor test. for that you have to either force creation in a before block or use let!.

palkan commented 2 years ago

Thanks for examples!

But who does before solves the problem? I though you wanted let to be reset between executions within a matcher; but since you're using the same record, you should be find without forcing the creation.

gagalago commented 2 years ago

if you don't put the before (or do let!(:page) { create(:page) } in the first line), then the page is created when we create the list of pages_works.

that list is created and then rollbacked in the database by populate for each scale factor, that means that the first scale factor will create the page (set the let value) and then remove the page from the database without changing the value contained by the let, the second scale factor will not create the page as the let still contain a value (cached by the previous) and will fail when he will try to create the list as there is no page in the database with the correct id. if I force the creation of the page before the populate, the populate will rollback only the list but not the page and so the second scale factor will still be able to use the page.

was it clearer?

palkan commented 2 years ago

Thanks for the explanation! Added an example to the docs.

Unfortunately, trying to automagically hack around RSpec memoization (let) could bring more problems, so we'd prefer to avoid this.