Commit 49f8697a authored by Grzegorz Bizon's avatar Grzegorz Bizon

Add additional test case for Gitaly N+1 for diff files

parent 356bf3af
...@@ -40,7 +40,7 @@ class EnvironmentStatus ...@@ -40,7 +40,7 @@ class EnvironmentStatus
end end
def changes def changes
return [] if project.route_map_for(sha).nil? return [] unless has_route_map?
changed_files.map { |file| build_change(file) }.compact changed_files.map { |file| build_change(file) }.compact
end end
...@@ -50,6 +50,10 @@ class EnvironmentStatus ...@@ -50,6 +50,10 @@ class EnvironmentStatus
.merge_request_diff_files.where(deleted_file: false) .merge_request_diff_files.where(deleted_file: false)
end end
def has_route_map?
project.route_map_for(sha).present?
end
private private
PAGE_EXTENSIONS = /\A\.(s?html?|php|asp|cgi|pl)\z/i.freeze PAGE_EXTENSIONS = /\A\.(s?html?|php|asp|cgi|pl)\z/i.freeze
......
...@@ -870,8 +870,13 @@ describe Projects::MergeRequestsController do ...@@ -870,8 +870,13 @@ describe Projects::MergeRequestsController do
end end
end end
context 'when multiple environments with deployments are present' do # we're trying to reduce the overall number of queries for this method.
let(:another_environment) { create(:environment, project: forked) } # set a hard limit for now. https://gitlab.com/gitlab-org/gitlab-ce/issues/52287
it 'keeps queries in check' do
control_count = ActiveRecord::QueryRecorder.new { get_ci_environments_status }.count
expect(control_count).to be <= 137
end
it 'has no N+1 SQL issues for environments', :request_store, retry: 0 do it 'has no N+1 SQL issues for environments', :request_store, retry: 0 do
# First run to insert test data from lets, which does take up some 30 queries # First run to insert test data from lets, which does take up some 30 queries
...@@ -879,10 +884,8 @@ describe Projects::MergeRequestsController do ...@@ -879,10 +884,8 @@ describe Projects::MergeRequestsController do
control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get_ci_environments_status }.count control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get_ci_environments_status }.count
create(:deployment, :succeed, environment: another_environment, environment2 = create(:environment, project: forked)
sha: sha, create(:deployment, :succeed, environment: environment2, sha: sha, ref: 'master', deployable: build)
ref: 'master',
deployable: build)
# TODO address the last 11 queries # TODO address the last 11 queries
# See https://gitlab.com/gitlab-org/gitlab-ce/issues/63952 (5 queries) # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63952 (5 queries)
...@@ -890,26 +893,53 @@ describe Projects::MergeRequestsController do ...@@ -890,26 +893,53 @@ describe Projects::MergeRequestsController do
leeway = 11 leeway = 11
expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count + leeway) expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count + leeway)
end end
end
it 'has no N+1 Gitaly requests for deployments', :request_store do context 'when a merge request has multiple environments with deployments' do
expect(merge_request).to be_present let(:sha) { merge_request.diff_head_sha }
let(:ref) { merge_request.source_branch }
let!(:build) { create(:ci_build, pipeline: pipeline) }
let!(:pipeline) { create(:ci_pipeline, sha: sha, project: project) }
let!(:environment) { create(:environment, name: 'env_a', project: project) }
let!(:another_environment) { create(:environment, name: 'env_b', project: project) }
before do
merge_request.update_head_pipeline
create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build)
create(:deployment, :succeed, environment: another_environment, sha: sha, ref: ref, deployable: build)
end
it 'exposes multiple environment statuses' do
get_ci_environments_status
create(:deployment, :succeed, environment: another_environment, expect(json_response.count).to eq 2
sha: sha, end
ref: 'master',
deployable: build) context 'when route map is not present in the project' do
it 'does not have N+1 Gitaly requests for environments', :request_store do
expect(merge_request).to be_present
expect { get_ci_environments_status } expect { get_ci_environments_status }
.not_to change { Gitlab::GitalyClient.get_request_count } .not_to change { Gitlab::GitalyClient.get_request_count }
end end
end end
# we're trying to reduce the overall number of queries for this method. context 'when there is route map present in a project' do
# set a hard limit for now. https://gitlab.com/gitlab-org/gitlab-ce/issues/52287 before do
it 'keeps queries in check' do allow_any_instance_of(EnvironmentStatus)
control_count = ActiveRecord::QueryRecorder.new { get_ci_environments_status }.count .to receive(:has_route_map?)
.and_return(true)
end
expect(control_count).to be <= 137 it 'does not have N+1 Gitaly requests for diff files', :request_store do
expect(merge_request.merge_request_diff.merge_request_diff_files).to be_many
expect { get_ci_environments_status }
.not_to change { Gitlab::GitalyClient.get_request_count }
end
end
end end
def get_ci_environments_status(extra_params = {}) def get_ci_environments_status(extra_params = {})
...@@ -923,7 +953,6 @@ describe Projects::MergeRequestsController do ...@@ -923,7 +953,6 @@ describe Projects::MergeRequestsController do
get :ci_environments_status, params: params.merge(extra_params) get :ci_environments_status, params: params.merge(extra_params)
end end
end end
end
describe 'GET pipeline_status.json' do describe 'GET pipeline_status.json' do
context 'when head_pipeline exists' do context 'when head_pipeline exists' do
......
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