Added unique identifier to calculate_reactive_cache. Decoupled comparison logic to service. Fixed N+1 select queries.

7 files changed, 102 insertions, 35 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index bf931a04592..59d533bd4d5 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -633,8 +633,6 @@ module Ci
     end
 
     def collect_test_reports!(test_reports)
-      raise ArgumentError, 'build does not have test reports' unless has_test_reports?
-
       test_reports.get_suite(group_name).tap do |test_suite|
         each_test_report do |file_type, blob|
           parse_test_report!(test_suite, file_type, blob)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 9f8ebd9b4ca..6de44751f1b 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1022,29 +1022,22 @@ class MergeRequest < ActiveRecord::Base
   end
 
   def compare_test_reports
-    unless actual_head_pipeline && actual_head_pipeline.has_test_reports?
-      return { status: :error, status_reason: 'head pipeline does not have test reports' }
+    unless has_test_reports?
+      return { status: :error, status_reason: 'This merge request does not have test reports' }
     end
 
-    with_reactive_cache(base_pipeline&.iid, actual_head_pipeline.iid) { |data| data } || { status: :parsing }
+    with_reactive_cache(
+      :compare_test_results,
+      base_pipeline&.iid,
+      actual_head_pipeline.iid) { |data| data } || { status: :parsing }
   end
 
-  def calculate_reactive_cache(base_pipeline_iid, head_pipeline_iid)
-    begin
-      base_pipeline = project.pipelines.find_by_iid(base_pipeline_iid)
-      head_pipeline = project.pipelines.find_by_iid(head_pipeline_iid)
-
-      comparer = Gitlab::Ci::Reports::TestReportsComparer
-        .new(base_pipeline&.test_reports, head_pipeline.test_reports)
-
-      {
-        status: :parsed,
-        data: TestReportsComparerSerializer
-          .new(project: project)
-          .represent(comparer).to_json
-      }
-    rescue => e
-      { status: :error, status_reason: e.message }
+  def calculate_reactive_cache(identifier, *args)
+    case identifier.to_sym
+    when :compare_test_results
+      Ci::CompareTestReportsService.new(project).execute(*args)
+    else
+      raise NotImplementedError, "Unknown identifier: #{identifier}"
     end
   end
 
diff --git a/app/services/ci/compare_test_reports_service.rb b/app/services/ci/compare_test_reports_service.rb
new file mode 100644
index 00000000000..8bc3354c403
--- /dev/null
+++ b/app/services/ci/compare_test_reports_service.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module Ci
+  class CompareTestReportsService < ::BaseService
+    def execute(base_pipeline_iid, head_pipeline_iid)
+      base_pipeline = project.pipelines.find_by_iid(base_pipeline_iid) if base_pipeline_iid
+      head_pipeline = project.pipelines.find_by_iid(head_pipeline_iid)
+
+      begin  
+        comparer = Gitlab::Ci::Reports::TestReportsComparer
+          .new(base_pipeline&.test_reports, head_pipeline.test_reports)
+  
+        {
+          status: :parsed,
+          data: TestReportsComparerSerializer
+            .new(project: project)
+            .represent(comparer).to_json
+        }
+      rescue => e
+        { status: :error, status_reason: e.message }
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/parsers/junit_parser.rb b/lib/gitlab/ci/parsers/junit_parser.rb
index 741c62707b0..68217c6b5c4 100644
--- a/lib/gitlab/ci/parsers/junit_parser.rb
+++ b/lib/gitlab/ci/parsers/junit_parser.rb
@@ -8,8 +8,10 @@ module Gitlab
 
         def initialize(xml_data)
           @data = Hash.from_xml(xml_data)
+        rescue  REXML::ParseException
+          raise JunitParserError, 'Failed to parse XML'
         rescue
-          raise JunitParserError, 'Invalid XML data'
+          raise JunitParserError, 'Unknown error'
         end
 
         def parse!(test_suite)
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 7137c07c55d..858e448a3d8 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -2845,7 +2845,7 @@ describe Ci::Build do
 
     context 'when build does not have test reports' do
       it 'raises an error' do
-        expect { subject }.to raise_error(ArgumentError)
+        expect { subject }.to raise_error(NoMethodError)
       end
     end
   end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 161c0b48626..d28250dbab7 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1127,7 +1127,9 @@ describe MergeRequest do
     end
 
     context 'when head pipeline has test reports' do
-      let!(:job_artifact) { create(:ci_job_artifact, :junit, job: head_pipeline.builds.first, project: project) }
+      before do
+        create(:ci_job_artifact, :junit, job: head_pipeline.builds.first, project: project)
+      end
 
       context 'when reactive cache worker is parsing asynchronously' do
         it 'returns status' do
@@ -1141,17 +1143,10 @@ describe MergeRequest do
         end
 
         it 'returns status and data' do
-          expect(subject[:status]).to eq(:parsed)
-          expect(subject[:data]).to be_a(String)
-        end
+          expect_any_instance_of(Ci::CompareTestReportsService)
+            .to receive(:execute).with(base_pipeline.iid, head_pipeline.iid)
 
-        context 'when test reports contains invalid data' do
-          let!(:job_artifact) { create(:ci_job_artifact, :junit_with_corrupted_data, job: head_pipeline.builds.first, project: project) }
-
-          it 'returns status and error message' do
-            expect(subject[:status]).to eq(:error)
-            expect(subject[:status_reason]).to eq('Invalid XML data')
-          end
+          subject
         end
       end
     end
@@ -1159,7 +1154,7 @@ describe MergeRequest do
     context 'when head pipeline does not have test reports' do
       it 'returns status and error message' do
         expect(subject[:status]).to eq(:error)
-        expect(subject[:status_reason]).to eq('head pipeline does not have test reports')
+        expect(subject[:status_reason]).to eq('This merge request does not have test reports')
       end
     end
   end
diff --git a/spec/services/ci/compare_test_reports_service_spec.rb b/spec/services/ci/compare_test_reports_service_spec.rb
new file mode 100644
index 00000000000..6278774f446
--- /dev/null
+++ b/spec/services/ci/compare_test_reports_service_spec.rb
@@ -0,0 +1,55 @@
+require 'spec_helper'
+
+describe Ci::CompareTestReportsService do
+  let(:service) { described_class.new(project) }
+  let(:project) { create(:project, :repository) }
+  let(:merge_request) { create(:merge_request, source_project: project) }
+
+  describe '#execute' do
+    subject { service.execute(base_pipeline.iid, head_pipeline.iid) }
+
+    let!(:base_pipeline) do
+      create(:ci_pipeline,
+        :success,
+        project: merge_request.source_project,
+        ref: merge_request.source_branch,
+        sha: merge_request.diff_base_sha).tap do |pipeline|
+        merge_request.update!(head_pipeline_id: pipeline.id)
+        create(:ci_build, name: 'rspec', pipeline: pipeline, project: project)
+      end
+    end
+
+    let!(:head_pipeline) do
+      create(:ci_pipeline,
+        :success,
+        project: merge_request.source_project,
+        ref: merge_request.source_branch,
+        sha: merge_request.diff_head_sha).tap do |pipeline|
+        merge_request.update!(head_pipeline_id: pipeline.id)
+        create(:ci_build, name: 'rspec', pipeline: pipeline, project: project)
+      end
+    end
+
+    context 'when head pipeline has test reports' do
+      before do
+        create(:ci_job_artifact, :junit, job: head_pipeline.builds.first, project: project)
+      end
+
+      it 'returns status and data' do
+        expect(subject[:status]).to eq(:parsed)
+        expect(subject[:data]).to match_schema('entities/test_reports_comparer')
+      end
+    end
+
+    context 'when head pipeline has corrupted test reports' do
+      before do
+        create(:ci_job_artifact, :junit_with_corrupted_data, job: head_pipeline.builds.first, project: project)
+      end
+
+      it 'returns status and error message' do
+        expect(subject[:status]).to eq(:error)
+        expect(subject[:status_reason]).to eq('Failed to parse XML')
+      end
+    end
+  end
+end