From 49f8697a4966affd27a49ee697dd4d72f6003e13 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 10 Jul 2019 16:07:14 +0200 Subject: Add additional test case for Gitaly N+1 for diff files --- app/models/environment_status.rb | 6 +- .../projects/merge_requests_controller_spec.rb | 101 +++++++++++++-------- 2 files changed, 70 insertions(+), 37 deletions(-) diff --git a/app/models/environment_status.rb b/app/models/environment_status.rb index 465a42759df..d7dc64190d6 100644 --- a/app/models/environment_status.rb +++ b/app/models/environment_status.rb @@ -40,7 +40,7 @@ class EnvironmentStatus end def changes - return [] if project.route_map_for(sha).nil? + return [] unless has_route_map? changed_files.map { |file| build_change(file) }.compact end @@ -50,6 +50,10 @@ class EnvironmentStatus .merge_request_diff_files.where(deleted_file: false) end + def has_route_map? + project.route_map_for(sha).present? + end + private PAGE_EXTENSIONS = /\A\.(s?html?|php|asp|cgi|pl)\z/i.freeze diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index b28fe48a705..e9d7fc8f909 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -870,58 +870,87 @@ describe Projects::MergeRequestsController do end end - context 'when multiple environments with deployments are present' do - let(:another_environment) { create(:environment, project: forked) } + # we're trying to reduce the overall number of queries for this method. + # 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 - 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 - get_ci_environments_status + expect(control_count).to be <= 137 + end - control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get_ci_environments_status }.count + 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 + get_ci_environments_status - create(:deployment, :succeed, environment: another_environment, - sha: sha, - ref: 'master', - deployable: build) + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get_ci_environments_status }.count - # TODO address the last 11 queries - # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63952 (5 queries) - # And https://gitlab.com/gitlab-org/gitlab-ce/issues/64105 (6 queries) - leeway = 11 - expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count + leeway) - end + environment2 = create(:environment, project: forked) + create(:deployment, :succeed, environment: environment2, sha: sha, ref: 'master', deployable: build) - it 'has no N+1 Gitaly requests for deployments', :request_store do - expect(merge_request).to be_present + # TODO address the last 11 queries + # See https://gitlab.com/gitlab-org/gitlab-ce/issues/63952 (5 queries) + # And https://gitlab.com/gitlab-org/gitlab-ce/issues/64105 (6 queries) + leeway = 11 + expect { get_ci_environments_status }.not_to exceed_all_query_limit(control_count + leeway) + end + end - create(:deployment, :succeed, environment: another_environment, - sha: sha, - ref: 'master', - deployable: build) + context 'when a merge request has multiple environments with deployments' do + 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 + + expect(json_response.count).to eq 2 + end + + 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 } .not_to change { Gitlab::GitalyClient.get_request_count } end end - # we're trying to reduce the overall number of queries for this method. - # 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 + context 'when there is route map present in a project' do + before do + allow_any_instance_of(EnvironmentStatus) + .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 - def get_ci_environments_status(extra_params = {}) - params = { - namespace_id: merge_request.project.namespace.to_param, - project_id: merge_request.project, - id: merge_request.iid, - format: 'json' - } + def get_ci_environments_status(extra_params = {}) + params = { + namespace_id: merge_request.project.namespace.to_param, + project_id: merge_request.project, + id: merge_request.iid, + format: 'json' + } - get :ci_environments_status, params: params.merge(extra_params) - end + get :ci_environments_status, params: params.merge(extra_params) end end -- cgit v1.2.1