Add factory for parsers. Add required specification in json schema matcher. Improved test code.

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

lib/gitlab/ci/parsers.rb

+module Gitlab
+  module Ci
+    module Parsers
+      def self.fabricate!(file_type)
+        "Gitlab::Ci::Parsers::#{file_type.classify}".constantize.new
+      end
+    end
+  end
+end

lib/gitlab/ci/parsers/junit_parser.rb → lib/gitlab/ci/parsers/junit.rb

 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

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
 

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

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
 

spec/fixtures/api/schemas/entities/test_case.json

 {
   "type": "object",
+  "required" : [
+    "status",
+    "name"
+  ],
   "properties": {
     "status": { "type": "string" },
     "name": { "type": "string" },

spec/fixtures/api/schemas/entities/test_reports_comparer.json

 {
   "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" } }
   },

spec/fixtures/api/schemas/entities/test_suite_comparer.json

 {
   "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" } },

spec/lib/gitlab/ci/parsers/junit_parser_spec.rb → spec/lib/gitlab/ci/parsers/junit_spec.rb

 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
-          <testsuite></testsuite>
-        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|

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) }

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
+    subject { service.execute(base_pipeline&.iid, head_pipeline.iid) }
+
+    context 'when head pipeline has test reports' do
+      let!(:base_pipeline) { nil }
+      let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) }
 
-    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)
+      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 test reports' do
-      before do
-        create(:ci_job_artifact, :junit, job: head_pipeline.builds.first, project: project)
-      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