-
Notifications
You must be signed in to change notification settings - Fork 830
AO3-4412 Adding a test to ensure there are not N+1 issues when loading inbox comments on the homepage #5913
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| require "spec_helper" | ||
|
|
||
| describe "n+1 queries in the inbox controller for the homepage" do | ||
| include LoginMacros | ||
|
|
||
| describe "#index", n_plus_one: true do | ||
| context "when displaying a user's unread messages on the homepage" do | ||
| let!(:user) { create(:user) } | ||
|
|
||
| subject do | ||
| proc do | ||
| get "/" | ||
| end | ||
| end | ||
|
|
||
| before do | ||
| fake_login_known_user(user) | ||
| end | ||
|
|
||
| context "when all works are cached" do | ||
| populate do |n| | ||
| create_list(:inbox_comment, n) | ||
| subject.call # this caches the comments | ||
| end | ||
|
|
||
| it "produces a constant number of queries" do | ||
| expect { subject.call } | ||
| .to perform_constant_number_of_queries | ||
| end | ||
| end | ||
|
|
||
| context "when nothing is cached" do | ||
| populate do |n| | ||
| create_list(:inbox_comment, n) | ||
| end | ||
|
|
||
| it "performs around 9 queries per message" do | ||
| # TODO: Ideally, we'd like the uncached inbox listings to also have a | ||
| # constant number of queries, instead of the linear number of queries | ||
| # we're checking for here. But we also don't want to put too much | ||
| # unnecessary load on the databases when we have a bunch of cache hits, | ||
| # so this requires some care & tuning. | ||
|
Comment on lines
+40
to
+42
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While this is the case for work blurbs, it's not the case for homepage inbox comments which aren't cached like work blurbs are. There are still a number of N+1 queries that can be avoided here and that we'd indeed like to avoid. You can refer to the very similar case of #5890 for some pointers of what queries to avoid (e.g. the role queries also show up in my logs for the homepage) and what kind of variants would be good to cover in the test |
||
| expect { subject.call } | ||
| .to perform_linear_number_of_queries(slope: 9).with_warming_up | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also for the other
create_list, otherwise this doesn't have any comments on the homepage for the userTo make sure the test fails in the future when it's not displaying more than one comment, could you add an
expectfor the number of comments after thesubject.call, similar to how it's done in the work tests withexpect(response.body.scan('<li id="work_').size).to eq(current_scale.to_i)?