Added unique identifier to calculate_reactive_cache. Decoupled comparison logic to service. Fixed N+1 select queries.
This commit is contained in:
parent
1f53cf7cf0
commit
85af3ea21a
|
@ -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)
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
|
@ -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
|
|
@ -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)
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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
|
Loading…
Reference in New Issue