From 41f28a9ffabf4eb45c53836ea4de3b7a49229eaa Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Fri, 3 Aug 2018 20:08:13 +0900 Subject: [PATCH] Add factory for parsers. Add required specification in json schema matcher. Improved test code. --- app/models/ci/build.rb | 7 +-- lib/gitlab/ci/parsers.rb | 9 ++++ .../ci/parsers/{junit_parser.rb => junit.rb} | 16 +++---- spec/factories/ci/builds.rb | 5 +- spec/factories/ci/pipelines.rb | 4 +- spec/factories/merge_requests.rb | 9 ++-- .../api/schemas/entities/test_case.json | 4 ++ .../entities/test_reports_comparer.json | 19 ++++++-- .../schemas/entities/test_suite_comparer.json | 22 +++++++-- .../{junit_parser_spec.rb => junit_spec.rb} | 42 ++++++----------- spec/models/merge_request_spec.rb | 37 +++++++-------- .../ci/compare_test_reports_service_spec.rb | 46 ++++++++----------- 12 files changed, 111 insertions(+), 109 deletions(-) create mode 100644 lib/gitlab/ci/parsers.rb rename lib/gitlab/ci/parsers/{junit_parser.rb => junit.rb} (81%) rename spec/lib/gitlab/ci/parsers/{junit_parser_spec.rb => junit_spec.rb} (83%) diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 59d533bd4d5..faa27216f28 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -635,7 +635,7 @@ module Ci def collect_test_reports!(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) + Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, test_suite) end end end @@ -650,11 +650,6 @@ module Ci end end - def parse_test_report!(test_suite, file_type, blob) - "Gitlab::Ci::Parsers::#{file_type.capitalize}Parser".constantize - .new(blob).parse!(test_suite) - end - def update_artifacts_size self.artifacts_size = legacy_artifacts_file&.size end diff --git a/lib/gitlab/ci/parsers.rb b/lib/gitlab/ci/parsers.rb new file mode 100644 index 00000000000..a4eccc08dfc --- /dev/null +++ b/lib/gitlab/ci/parsers.rb @@ -0,0 +1,9 @@ +module Gitlab + module Ci + module Parsers + def self.fabricate!(file_type) + "Gitlab::Ci::Parsers::#{file_type.classify}".constantize.new + end + end + end +end diff --git a/lib/gitlab/ci/parsers/junit_parser.rb b/lib/gitlab/ci/parsers/junit.rb similarity index 81% rename from lib/gitlab/ci/parsers/junit_parser.rb rename to lib/gitlab/ci/parsers/junit.rb index 68217c6b5c4..3c4668ec13b 100644 --- a/lib/gitlab/ci/parsers/junit_parser.rb +++ b/lib/gitlab/ci/parsers/junit.rb @@ -1,28 +1,24 @@ module Gitlab module Ci module Parsers - class JunitParser + class Junit attr_reader :data JunitParserError = Class.new(StandardError) - def initialize(xml_data) + def parse!(xml_data, test_suite) @data = Hash.from_xml(xml_data) - rescue REXML::ParseException - raise JunitParserError, 'Failed to parse XML' - rescue - raise JunitParserError, 'Unknown error' - end - def parse!(test_suite) each_suite do |testcases| testcases.each do |testcase| test_case = create_test_case(testcase) test_suite.add_test_case(test_case) end end - rescue - raise JunitParserError, 'Invalid JUnit xml structure' + rescue REXML::ParseException => e + raise JunitParserError, "XML parsing failed: #{e.message}" + rescue => e + raise JunitParserError, "JUnit parsing failed: #{e.message}" end private diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index 8bd1f1ae4e0..9813190925b 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -188,9 +188,8 @@ FactoryBot.define do end trait :test_reports do - after(:create) do |build| - create(:ci_job_artifact, :junit, job: build) - build.reload + after(:build) do |build| + build.job_artifacts << create(:ci_job_artifact, :junit, job: build) end end diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index 457fcbad620..a6ff226fa75 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -70,11 +70,11 @@ FactoryBot.define do protected true end - trait :test_reports do + trait :with_test_reports do status :success after(:build) do |pipeline, evaluator| - create(:ci_build, :test_reports, pipeline: pipeline, project: pipeline.project) + pipeline.builds << build(:ci_build, :test_reports, pipeline: pipeline, project: pipeline.project) end end end diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index 4c36ec8ec58..3268b607a76 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -90,15 +90,14 @@ FactoryBot.define do end trait :with_test_reports do - after(:create) do |merge_request| - create(:ci_pipeline, + after(:build) do |merge_request| + merge_request.head_pipeline = build( + :ci_pipeline, :success, :test_reports, 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) - end + sha: merge_request.diff_head_sha) end end diff --git a/spec/fixtures/api/schemas/entities/test_case.json b/spec/fixtures/api/schemas/entities/test_case.json index 9b08d6c2302..c9ba1f3ad18 100644 --- a/spec/fixtures/api/schemas/entities/test_case.json +++ b/spec/fixtures/api/schemas/entities/test_case.json @@ -1,5 +1,9 @@ { "type": "object", + "required" : [ + "status", + "name" + ], "properties": { "status": { "type": "string" }, "name": { "type": "string" }, diff --git a/spec/fixtures/api/schemas/entities/test_reports_comparer.json b/spec/fixtures/api/schemas/entities/test_reports_comparer.json index a7cd934069a..d7880801c01 100644 --- a/spec/fixtures/api/schemas/entities/test_reports_comparer.json +++ b/spec/fixtures/api/schemas/entities/test_reports_comparer.json @@ -1,11 +1,24 @@ { "type": "object", + "required" : [ + "status", + "summary", + "suites" + ], "properties": { "status": { "type": "string" }, "summary": { - "total": { "type": "integer" }, - "resolved": { "type": "integer" }, - "failed": { "type": "integer" } + "type": "object", + "properties": { + "total": { "type": "integer" }, + "resolved": { "type": "integer" }, + "failed": { "type": "integer" } + }, + "required": [ + "total", + "resolved", + "failed" + ] }, "suites": { "type": "array", "items": { "$ref": "test_suite_comparer.json" } } }, diff --git a/spec/fixtures/api/schemas/entities/test_suite_comparer.json b/spec/fixtures/api/schemas/entities/test_suite_comparer.json index 192a8f3fd71..d63fea1f0db 100644 --- a/spec/fixtures/api/schemas/entities/test_suite_comparer.json +++ b/spec/fixtures/api/schemas/entities/test_suite_comparer.json @@ -1,12 +1,28 @@ { "type": "object", + "required": [ + "name", + "status", + "summary", + "new_failures", + "resolved_failures", + "existing_failures" + ], "properties": { "name": { "type": "string" }, "status": { "type": "string" }, "summary": { - "total": { "type": "integer" }, - "resolved": { "type": "integer" }, - "failed": { "type": "integer" } + "type": "object", + "properties": { + "total": { "type": "integer" }, + "resolved": { "type": "integer" }, + "failed": { "type": "integer" } + }, + "required": [ + "total", + "resolved", + "failed" + ] }, "new_failures": { "type": "array", "items": { "$ref": "test_case.json" } }, "resolved_failures": { "type": "array", "items": { "$ref": "test_case.json" } }, diff --git a/spec/lib/gitlab/ci/parsers/junit_parser_spec.rb b/spec/lib/gitlab/ci/parsers/junit_spec.rb similarity index 83% rename from spec/lib/gitlab/ci/parsers/junit_parser_spec.rb rename to spec/lib/gitlab/ci/parsers/junit_spec.rb index 5459fb96610..f7ec86f5385 100644 --- a/spec/lib/gitlab/ci/parsers/junit_parser_spec.rb +++ b/spec/lib/gitlab/ci/parsers/junit_spec.rb @@ -1,39 +1,13 @@ require 'spec_helper' -describe Gitlab::Ci::Parsers::JunitParser do - describe '#initialize' do - context 'when xml data is given' do - let(:data) do - <<-EOF.strip_heredoc - - EOF - end - - let(:parser) { described_class.new(data) } - - it 'initialize Hash from the given data' do - expect { parser }.not_to raise_error - - expect(parser.data).to be_a(Hash) - end - end - - context 'when json data is given' do - let(:data) { { testsuite: 'abc' }.to_json } - - it 'raises an error' do - expect { described_class.new(data) }.to raise_error(described_class::JunitParserError) - end - end - end - +describe Gitlab::Ci::Parsers::Junit do describe '#parse!' do - subject { described_class.new(junit).parse!(test_suite) } + subject { described_class.new.parse!(junit, test_suite) } let(:test_suite) { Gitlab::Ci::Reports::TestSuite.new('rspec') } let(:test_cases) { flattened_test_cases(test_suite) } - context 'when XML is formated as JUnit' do + context 'when data is JUnit style XML' do context 'when there are no test cases' do let(:junit) do <<-EOF.strip_heredoc @@ -123,6 +97,16 @@ describe Gitlab::Ci::Parsers::JunitParser do end end + context 'when data is not JUnit style XML' do + let(:junit) { { testsuite: 'abc' }.to_json } + + it 'raises an error' do + expect { subject }.to raise_error(described_class::JunitParserError) + end + end + + private + def flattened_test_cases(test_suite) test_suite.test_cases.map do |status, value| value.map do |key, test_case| diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index d28250dbab7..1223cf6a919 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1105,30 +1105,20 @@ describe MergeRequest do let(:merge_request) { create(:merge_request, source_project: project) } 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 + create(:ci_pipeline, :with_test_reports, project: project, ref: merge_request.target_branch, sha: merge_request.diff_base_sha) 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 + before do + merge_request.update!(head_pipeline_id: head_pipeline.id) end context 'when head pipeline has test reports' do - before do - create(:ci_job_artifact, :junit, job: head_pipeline.builds.first, project: project) + let!(:head_pipeline) do + create(:ci_pipeline, + :with_test_reports, + project: project, + ref: merge_request.source_branch, + sha: merge_request.diff_head_sha) end context 'when reactive cache worker is parsing asynchronously' do @@ -1152,6 +1142,13 @@ describe MergeRequest do end context 'when head pipeline does not have test reports' do + let!(:head_pipeline) do + create(:ci_pipeline, + project: project, + ref: merge_request.source_branch, + sha: merge_request.diff_head_sha) + end + it 'returns status and error message' do expect(subject[:status]).to eq(:error) expect(subject[:status_reason]).to eq('This merge request does not have test reports') @@ -2099,7 +2096,7 @@ describe MergeRequest do } end - let(:project) { create(:project, :public, :repository) } + let(:project) { create(:project, :public, :repository) } let(:merge_request) { create(:merge_request, source_project: project) } let!(:first_pipeline) { create(:ci_pipeline_without_jobs, pipeline_arguments) } diff --git a/spec/services/ci/compare_test_reports_service_spec.rb b/spec/services/ci/compare_test_reports_service_spec.rb index 6278774f446..d3bbf17cc5c 100644 --- a/spec/services/ci/compare_test_reports_service_spec.rb +++ b/spec/services/ci/compare_test_reports_service_spec.rb @@ -3,37 +3,23 @@ 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 + subject { service.execute(base_pipeline&.iid, head_pipeline.iid) } context 'when head pipeline has test reports' do - before do - create(:ci_job_artifact, :junit, job: head_pipeline.builds.first, project: project) + let!(:base_pipeline) { nil } + let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } + + 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 base and head pipelines have test reports' do + let!(:base_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } + let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) } it 'returns status and data' do expect(subject[:status]).to eq(:parsed) @@ -42,13 +28,17 @@ describe Ci::CompareTestReportsService do end context 'when head pipeline has corrupted test reports' do + let!(:base_pipeline) { nil } + let!(:head_pipeline) { create(:ci_pipeline, project: project) } + before do - create(:ci_job_artifact, :junit_with_corrupted_data, job: head_pipeline.builds.first, project: project) + build = create(:ci_build, pipeline: head_pipeline, project: head_pipeline.project) + create(:ci_job_artifact, :junit_with_corrupted_data, job: build, 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') + expect(subject[:status_reason]).to include('XML parsing failed') end end end