Commit c5907053 authored by Gary Holtz's avatar Gary Holtz Committed by Matthias Käppler

Fix N+1 spec to test correctly

Move it to request spec since we are transitioning controller
specs to request specs. Also, query counter should be used in
request specs as it is possible to return false positives when
used in controller specs.
parent 72b6be71
...@@ -9,7 +9,7 @@ module EE ...@@ -9,7 +9,7 @@ module EE
def preload_for_collection def preload_for_collection
@preload_for_collection ||= case collection_type @preload_for_collection ||= case collection_type
when 'MergeRequest' when 'MergeRequest'
super.push(:approval_rules) super + [approval_rules: [:users, :group_users], approval_project_rules: [:users, :group_users]]
when 'Issue' when 'Issue'
super.push(*issue_preloads) super.push(*issue_preloads)
else else
......
---
title: Resolve N+1 issue loading approval rules on the MR list
merge_request: 52364
author:
type: performance
...@@ -60,39 +60,6 @@ RSpec.describe Projects::MergeRequestsController do ...@@ -60,39 +60,6 @@ RSpec.describe Projects::MergeRequestsController do
sign_in(viewer) sign_in(viewer)
end end
describe 'GET index' do
def get_merge_requests
get :index,
params: {
namespace_id: project.namespace.to_param,
project_id: project,
state: 'opened'
}
end
context 'when filtering by opened state' do
context 'with opened merge requests' do
render_views
it 'avoids N+1' do
other_user = create(:user)
create(:merge_request, :unique_branches, target_project: project, source_project: project)
create_list(:approval_merge_request_rule, 5, merge_request: merge_request, users: [user, other_user], approvals_required: 2)
control_count = ActiveRecord::QueryRecorder.new { get_merge_requests }.count
create_list(:approval, 10)
create_list(:merge_request, 20, :unique_branches, target_project: project, source_project: project).each do |mr|
create(:approval_merge_request_rule, merge_request: merge_request, users: [user, other_user], approvals_required: 2)
end
expect do
get_merge_requests
end.not_to exceed_query_limit(control_count)
end
end
end
end
describe 'PUT update' do describe 'PUT update' do
let_it_be_with_reload(:merge_request) do let_it_be_with_reload(:merge_request) do
create(:merge_request_with_diffs, source_project: project, author: author) create(:merge_request_with_diffs, source_project: project, author: author)
......
...@@ -43,4 +43,27 @@ RSpec.describe Projects::MergeRequestsController do ...@@ -43,4 +43,27 @@ RSpec.describe Projects::MergeRequestsController do
end end
end end
end end
describe 'GET #index' do
def get_index
get project_merge_requests_path(project, state: 'opened')
end
it 'avoids N+1' do
other_user = create(:user)
create(:merge_request, :unique_branches, target_project: project, source_project: project)
create_list(:approval_project_rule, 5, project: project, users: [user, other_user], approvals_required: 2)
create_list(:approval_merge_request_rule, 5, merge_request: merge_request, users: [user, other_user], approvals_required: 2)
control_count = ActiveRecord::QueryRecorder.new { get_index }.count
create_list(:approval, 10)
create(:approval_project_rule, project: project, users: [user, other_user], approvals_required: 2)
create_list(:merge_request, 20, :unique_branches, target_project: project, source_project: project).each do |mr|
create(:approval_merge_request_rule, merge_request: mr, users: [user, other_user], approvals_required: 2)
end
expect { get_index }.not_to exceed_query_limit(control_count)
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment