Merge branch '7788_parse_license_management_reports_in_be-ce' into 'master'

Generalise test compare service

See merge request gitlab-org/gitlab-ce!22833

10 files changed, 126 insertions, 68 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index d86a6eceb59..e2917049902 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -742,7 +742,7 @@ module Ci
     def collect_test_reports!(test_reports)
       test_reports.get_suite(group_name).tap do |test_suite|
         each_report(Ci::JobArtifact::TEST_REPORT_FILE_TYPES) do |file_type, blob|
-          Gitlab::Ci::Parsers::Test.fabricate!(file_type).parse!(blob, test_suite)
+          Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, test_suite)
         end
       end
     end
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 77e48ce11e8..a13cac73d04 100644
--- a/app/models/merge_request.rb
+++ b/app/models/merge_request.rb
@@ -1118,14 +1118,17 @@ class MergeRequest < ActiveRecord::Base
     end
   end
 
-  # rubocop: disable CodeReuse/ServiceClass
   def compare_test_reports
     unless has_test_reports?
       return { status: :error, status_reason: 'This merge request does not have test reports' }
     end
 
-    with_reactive_cache(:compare_test_results) do |data|
-      unless Ci::CompareTestReportsService.new(project)
+    compare_reports(Ci::CompareTestReportsService)
+  end
+
+  def compare_reports(service_class)
+    with_reactive_cache(service_class.name) do |data|
+      unless service_class.new(project)
         .latest?(base_pipeline, actual_head_pipeline, data)
         raise InvalidateReactiveCache
       end
@@ -1133,19 +1136,14 @@ class MergeRequest < ActiveRecord::Base
       data
     end || { status: :parsing }
   end
-  # rubocop: enable CodeReuse/ServiceClass
 
-  # rubocop: disable CodeReuse/ServiceClass
   def calculate_reactive_cache(identifier, *args)
-    case identifier.to_sym
-    when :compare_test_results
-      Ci::CompareTestReportsService.new(project).execute(
-        base_pipeline, actual_head_pipeline)
-    else
-      raise NotImplementedError, "Unknown identifier: #{identifier}"
-    end
+    service_class = identifier.constantize
+
+    raise NameError, service_class unless service_class < Ci::CompareReportsBaseService
+
+    service_class.new(project).execute(base_pipeline, actual_head_pipeline)
   end
-  # rubocop: enable CodeReuse/ServiceClass
 
   def all_commits
     # MySQL doesn't support LIMIT in a subquery.
diff --git a/app/services/ci/compare_reports_base_service.rb b/app/services/ci/compare_reports_base_service.rb
new file mode 100644
index 00000000000..d5625857599
--- /dev/null
+++ b/app/services/ci/compare_reports_base_service.rb
@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+
+module Ci
+  class CompareReportsBaseService < ::BaseService
+    def execute(base_pipeline, head_pipeline)
+      comparer = comparer_class.new(get_report(base_pipeline), get_report(head_pipeline))
+      {
+        status: :parsed,
+        key: key(base_pipeline, head_pipeline),
+        data: serializer_class
+          .new(project: project)
+          .represent(comparer).as_json
+      }
+    rescue Gitlab::Ci::Parsers::ParserError => e
+      {
+        status: :error,
+        key: key(base_pipeline, head_pipeline),
+        status_reason: e.message
+      }
+    end
+
+    def latest?(base_pipeline, head_pipeline, data)
+      data&.fetch(:key, nil) == key(base_pipeline, head_pipeline)
+    end
+
+    private
+
+    def key(base_pipeline, head_pipeline)
+      [
+        base_pipeline&.id, base_pipeline&.updated_at,
+        head_pipeline&.id, head_pipeline&.updated_at
+      ]
+    end
+
+    def comparer_class
+      raise NotImplementedError
+    end
+
+    def serializer_class
+      raise NotImplementedError
+    end
+
+    def get_report(pipeline)
+      raise NotImplementedError
+    end
+  end
+end
diff --git a/app/services/ci/compare_test_reports_service.rb b/app/services/ci/compare_test_reports_service.rb
index 2293f95f56b..382d5b8995f 100644
--- a/app/services/ci/compare_test_reports_service.rb
+++ b/app/services/ci/compare_test_reports_service.rb
@@ -1,39 +1,17 @@
 # frozen_string_literal: true
 
 module Ci
-  class CompareTestReportsService < ::BaseService
-    def execute(base_pipeline, head_pipeline)
-      # rubocop: disable CodeReuse/Serializer
-      comparer = Gitlab::Ci::Reports::TestReportsComparer
-        .new(base_pipeline&.test_reports, head_pipeline.test_reports)
-
-      {
-        status: :parsed,
-        key: key(base_pipeline, head_pipeline),
-        data: TestReportsComparerSerializer
-          .new(project: project)
-          .represent(comparer).as_json
-      }
-    rescue => e
-      {
-        status: :error,
-        key: key(base_pipeline, head_pipeline),
-        status_reason: e.message
-      }
-      # rubocop: enable CodeReuse/Serializer
+  class CompareTestReportsService < CompareReportsBaseService
+    def comparer_class
+      Gitlab::Ci::Reports::TestReportsComparer
     end
 
-    def latest?(base_pipeline, head_pipeline, data)
-      data&.fetch(:key, nil) == key(base_pipeline, head_pipeline)
+    def serializer_class
+      TestReportsComparerSerializer
     end
 
-    private
-
-    def key(base_pipeline, head_pipeline)
-      [
-        base_pipeline&.id, base_pipeline&.updated_at,
-        head_pipeline&.id, head_pipeline&.updated_at
-      ]
+    def get_report(pipeline)
+      pipeline&.test_reports
     end
   end
 end
diff --git a/lib/gitlab/ci/parsers.rb b/lib/gitlab/ci/parsers.rb
new file mode 100644
index 00000000000..eb63e6c8363
--- /dev/null
+++ b/lib/gitlab/ci/parsers.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Parsers
+      ParserNotFoundError = Class.new(ParserError)
+
+      def self.parsers
+        {
+          junit: ::Gitlab::Ci::Parsers::Test::Junit
+        }
+      end
+
+      def self.fabricate!(file_type)
+        parsers.fetch(file_type.to_sym).new
+      rescue KeyError
+        raise ParserNotFoundError, "Cannot find any parser matching file type '#{file_type}'"
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/ci/parsers/parser_error.rb b/lib/gitlab/ci/parsers/parser_error.rb
new file mode 100644
index 00000000000..ef327737cdb
--- /dev/null
+++ b/lib/gitlab/ci/parsers/parser_error.rb
@@ -0,0 +1,9 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Parsers
+      ParserError = Class.new(StandardError)
+    end
+  end
+end
diff --git a/lib/gitlab/ci/parsers/test.rb b/lib/gitlab/ci/parsers/test.rb
deleted file mode 100644
index c6bc9662b07..00000000000
--- a/lib/gitlab/ci/parsers/test.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-module Gitlab
-  module Ci
-    module Parsers
-      module Test
-        ParserNotFoundError = Class.new(StandardError)
-
-        PARSERS = {
-          junit: ::Gitlab::Ci::Parsers::Test::Junit
-        }.freeze
-
-        def self.fabricate!(file_type)
-          PARSERS.fetch(file_type.to_sym).new
-        rescue KeyError
-          raise ParserNotFoundError, "Cannot find any parser matching file type '#{file_type}'"
-        end
-      end
-    end
-  end
-end
diff --git a/lib/gitlab/ci/parsers/test/junit.rb b/lib/gitlab/ci/parsers/test/junit.rb
index 2791730fd26..dca60eabc1c 100644
--- a/lib/gitlab/ci/parsers/test/junit.rb
+++ b/lib/gitlab/ci/parsers/test/junit.rb
@@ -5,7 +5,7 @@ module Gitlab
     module Parsers
       module Test
         class Junit
-          JunitParserError = Class.new(StandardError)
+          JunitParserError = Class.new(Gitlab::Ci::Parsers::ParserError)
 
           def parse!(xml_data, test_suite)
             root = Hash.from_xml(xml_data)
diff --git a/spec/lib/gitlab/ci/parsers/test_spec.rb b/spec/lib/gitlab/ci/parsers_spec.rb
index 0b85b432677..4b647bffe59 100644
--- a/spec/lib/gitlab/ci/parsers/test_spec.rb
+++ b/spec/lib/gitlab/ci/parsers_spec.rb
@@ -1,6 +1,8 @@
+# frozen_string_literal: true
+
 require 'spec_helper'
 
-describe Gitlab::Ci::Parsers::Test do
+describe Gitlab::Ci::Parsers do
   describe '.fabricate!' do
     subject { described_class.fabricate!(file_type) }
 
@@ -8,7 +10,7 @@ describe Gitlab::Ci::Parsers::Test do
       let(:file_type) { 'junit' }
 
       it 'fabricates the class' do
-        is_expected.to be_a(described_class::Junit)
+        is_expected.to be_a(described_class::Test::Junit)
       end
     end
 
@@ -16,7 +18,7 @@ describe Gitlab::Ci::Parsers::Test do
       let(:file_type) { 'undefined' }
 
       it 'raises an error' do
-        expect { subject }.to raise_error(Gitlab::Ci::Parsers::Test::ParserNotFoundError)
+        expect { subject }.to raise_error(Gitlab::Ci::Parsers::ParserNotFoundError)
       end
     end
   end
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index c3152d2021b..bf4117fbcaf 100644
--- a/spec/models/merge_request_spec.rb
+++ b/spec/models/merge_request_spec.rb
@@ -1339,6 +1339,30 @@ describe MergeRequest do
     end
   end
 
+  describe '#calculate_reactive_cache' do
+    let(:project) { create(:project, :repository) }
+    let(:merge_request) { create(:merge_request, source_project: project) }
+    subject { merge_request.calculate_reactive_cache(service_class_name) }
+
+    context 'when given an unknown service class name' do
+      let(:service_class_name) { 'Integer' }
+
+      it 'raises a NameError exception' do
+        expect { subject }.to raise_error(NameError, service_class_name)
+      end
+    end
+
+    context 'when given a known service class name' do
+      let(:service_class_name) { 'Ci::CompareTestReportsService' }
+
+      it 'does not raises a NameError exception' do
+        allow_any_instance_of(service_class_name.constantize).to receive(:execute).and_return(nil)
+
+        expect { subject }.not_to raise_error(NameError)
+      end
+    end
+  end
+
   describe '#compare_test_reports' do
     subject { merge_request.compare_test_reports }