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

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