diff options
author | Shinya Maeda <shinya@gitlab.com> | 2018-08-03 09:13:39 +0300 |
---|---|---|
committer | Shinya Maeda <shinya@gitlab.com> | 2018-08-03 09:13:39 +0300 |
commit | 85af3ea21a5a772cb22756190268ba3f921a6400 (patch) | |
tree | 4231bb48df2693c9600b5c5b7dabb9bae41c1d3e | |
parent | 1f53cf7cf0cb53b5d69ab141fa9020356e62027e (diff) |
Added unique identifier to calculate_reactive_cache. Decoupled comparison logic to service. Fixed N+1 select queries.
-rw-r--r-- | app/models/ci/build.rb | 2 | ||||
-rw-r--r-- | app/models/merge_request.rb | 31 | ||||
-rw-r--r-- | app/services/ci/compare_test_reports_service.rb | 24 | ||||
-rw-r--r-- | lib/gitlab/ci/parsers/junit_parser.rb | 4 | ||||
-rw-r--r-- | spec/models/ci/build_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/merge_request_spec.rb | 19 | ||||
-rw-r--r-- | spec/services/ci/compare_test_reports_service_spec.rb | 55 |
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 |