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 similarity index 64% rename from spec/lib/gitlab/ci/parsers/test_spec.rb rename to 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 }