Merge branch 'remove-exceptions-from-junit-parser' into 'master'

Remove exception raising from JUnit parser

See merge request gitlab-org/gitlab!24188

app/assets/javascripts/pipelines/components/test_reports/test_reports.vue

@@ -21,7 +21,8 @@ export default {
       return this.selectedSuite.total_count > 0;
     },
     showTests() {
-      return this.testReports.total_count > 0;
+      const { test_suites: testSuites = [] } = this.testReports;
+      return testSuites.length > 0;
     },
   },
   methods: {

app/assets/javascripts/pipelines/components/test_reports/test_summary_table.vue

 <script>
 import { mapGetters } from 'vuex';
 import { s__ } from '~/locale';
+import { GlIcon, GlTooltipDirective } from '@gitlab/ui';
 import store from '~/pipelines/stores/test_reports';
 import SmartVirtualList from '~/vue_shared/components/smart_virtual_list.vue';
 
 export default {
   name: 'TestsSummaryTable',
   components: {
+    GlIcon,
     SmartVirtualList,
   },
+  directives: {
+    GlTooltip: GlTooltipDirective,
+  },
   store,
   props: {
     heading: {
@@ -75,7 +80,10 @@ export default {
           v-for="(testSuite, index) in getTestSuites"
           :key="index"
           role="row"
-          class="gl-responsive-table-row gl-responsive-table-row-clickable test-reports-summary-row rounded cursor-pointer js-suite-row"
+          class="gl-responsive-table-row test-reports-summary-row rounded js-suite-row"
+          :class="{
+            'gl-responsive-table-row-clickable cursor-pointer': !testSuite.suite_error,
+          }"
           @click="tableRowClick(testSuite)"
         >
           <div class="table-section section-25">
@@ -84,6 +92,14 @@ export default {
             </div>
             <div class="table-mobile-content underline cgray pl-3">
               {{ testSuite.name }}
+              <gl-icon
+                v-if="testSuite.suite_error"
+                ref="suiteErrorIcon"
+                v-gl-tooltip
+                name="error"
+                :title="testSuite.suite_error"
+                class="vertical-align-middle"
+              />
             </div>
           </div>
 

app/assets/javascripts/reports/components/grouped_test_reports_app.vue

 <script>
 import { mapActions, mapGetters, mapState } from 'vuex';
-import { s__ } from '~/locale';
+import { sprintf, s__ } from '~/locale';
 import { componentNames } from './issue_body';
 import ReportSection from './report_section.vue';
 import SummaryRow from './summary_row.vue';
@@ -52,8 +52,17 @@ export default {
   methods: {
     ...mapActions(['setEndpoint', 'fetchReports']),
     reportText(report) {
-      const summary = report.summary || {};
-      return reportTextBuilder(report.name, summary);
+      const { name, summary } = report || {};
+
+      if (report.status === 'error') {
+        return sprintf(s__('Reports|An error occurred while loading %{name} results'), { name });
+      }
+
+      if (!report.name) {
+        return s__('Reports|An error occured while loading report');
+      }
+
+      return reportTextBuilder(name, summary);
     },
     getReportIcon(report) {
       return statusIcon(report.status);

app/assets/javascripts/reports/store/mutations.js

@@ -8,8 +8,7 @@ export default {
     state.isLoading = true;
   },
   [types.RECEIVE_REPORTS_SUCCESS](state, response) {
-    // Make sure to clean previous state in case it was an error
-    state.hasError = false;
+    state.hasError = response.suites.some(suite => suite.status === 'error');
 
     state.isLoading = false;
 

app/controllers/projects/pipelines_controller.rb

@@ -169,19 +169,9 @@ class Projects::PipelinesController < Projects::ApplicationController
       end
 
       format.json do
-        if pipeline_test_report == :error
-          render json: { status: :error_parsing_report }
-        else
-          test_reports = if params[:scope] == "with_attachment"
-                           pipeline_test_report.with_attachment!
-                         else
-                           pipeline_test_report
-                         end
-
-          render json: TestReportSerializer
-            .new(current_user: @current_user)
-            .represent(test_reports, project: project)
-        end
+        render json: TestReportSerializer
+          .new(current_user: @current_user)
+          .represent(pipeline_test_report, project: project)
       end
     end
   end
@@ -189,11 +179,7 @@ class Projects::PipelinesController < Projects::ApplicationController
   def test_reports_count
     return unless Feature.enabled?(:junit_pipeline_view, project)
 
-    begin
-      render json: { total_count: pipeline.test_reports_count }.to_json
-    rescue Gitlab::Ci::Parsers::ParserError
-      render json: { total_count: 0 }.to_json
-    end
+    render json: { total_count: pipeline.test_reports_count }.to_json
   end
 
   private
@@ -269,9 +255,9 @@ class Projects::PipelinesController < Projects::ApplicationController
 
   def pipeline_test_report
     strong_memoize(:pipeline_test_report) do
-      @pipeline.test_reports
-    rescue Gitlab::Ci::Parsers::ParserError
-      :error
+      @pipeline.test_reports.tap do |reports|
+        reports.with_attachment! if params[:scope] == 'with_attachment'
+      end
     end
   end
 end

app/serializers/test_suite_entity.rb

@@ -9,8 +9,9 @@ class TestSuiteEntity < Grape::Entity
   expose :failed_count
   expose :skipped_count
   expose :error_count
+  expose :suite_error
 
   expose :test_cases, using: TestCaseEntity do |test_suite|
-    test_suite.test_cases.values.flat_map(&:values)
+    test_suite.suite_error ? [] : test_suite.test_cases.values.flat_map(&:values)
   end
 end

changelogs/unreleased/remove-exceptions-from-junit-parser.yml

+---
+title: Render TestReport parsing errors back to pipeline test summary
+merge_request: 24188
+author:
+type: fixed

lib/gitlab/ci/parsers/test/junit.rb

@@ -15,10 +15,10 @@ module Gitlab
               test_case = create_test_case(test_case, args)
               test_suite.add_test_case(test_case)
             end
-          rescue Nokogiri::XML::SyntaxError
-            raise JunitParserError, "XML parsing failed"
-          rescue
-            raise JunitParserError, "JUnit parsing failed"
+          rescue Nokogiri::XML::SyntaxError => e
+            test_suite.set_suite_error("JUnit XML parsing failed: #{e}")
+          rescue StandardError => e
+            test_suite.set_suite_error("JUnit data parsing failed: #{e}")
           end
 
           private

lib/gitlab/ci/reports/test_reports.rb

@@ -42,6 +42,12 @@ module Gitlab
           self
         end
 
+        def suite_errors
+          test_suites.each_with_object({}) do |(name, suite), errors|
+            errors[suite.name] = suite.suite_error if suite.suite_error
+          end
+        end
+
         TestCase::STATUS_TYPES.each do |status_type|
           define_method("#{status_type}_count") do
             # rubocop: disable CodeReuse/ActiveRecord

lib/gitlab/ci/reports/test_suite.rb

@@ -7,6 +7,7 @@ module Gitlab
         attr_reader :name
         attr_reader :test_cases
         attr_reader :total_time
+        attr_reader :suite_error
 
         def initialize(name = nil)
           @name = name
@@ -25,12 +26,16 @@ module Gitlab
 
         # rubocop: disable CodeReuse/ActiveRecord
         def total_count
+          return 0 if suite_error
+
           test_cases.values.sum(&:count)
         end
         # rubocop: enable CodeReuse/ActiveRecord
 
         def total_status
-          if failed_count > 0 || error_count > 0
+          if suite_error
+            TestCase::STATUS_ERROR
+          elsif failed_count > 0 || error_count > 0
             TestCase::STATUS_FAILED
           else
             TestCase::STATUS_SUCCESS
@@ -49,14 +54,22 @@ module Gitlab
 
         TestCase::STATUS_TYPES.each do |status_type|
           define_method("#{status_type}") do
-            test_cases[status_type] || {}
+            return {} if suite_error || test_cases[status_type].nil?
+
+            test_cases[status_type]
           end
 
           define_method("#{status_type}_count") do
-            test_cases[status_type]&.length.to_i
+            return 0 if suite_error || test_cases[status_type].nil?
+
+            test_cases[status_type].length
           end
         end
 
+        def set_suite_error(msg)
+          @suite_error = msg
+        end
+
         private
 
         def existing_key?(test_case)

locale/gitlab.pot

@@ -17222,6 +17222,12 @@ msgstr ""
 msgid "Reports|Actions"
 msgstr ""
 
+msgid "Reports|An error occured while loading report"
+msgstr ""
+
+msgid "Reports|An error occurred while loading %{name} results"
+msgstr ""
+
 msgid "Reports|Class"
 msgstr ""
 

spec/controllers/projects/pipelines_controller_spec.rb

@@ -752,8 +752,6 @@ describe Projects::PipelinesController do
       end
 
       context 'when pipeline does not have a test report' do
-        let(:pipeline) { create(:ci_pipeline, project: project) }
-
         it 'renders an empty test report' do
           get_test_report_json
 
@@ -763,7 +761,11 @@ describe Projects::PipelinesController do
       end
 
       context 'when pipeline has a test report' do
-        let(:pipeline) { create(:ci_pipeline, :with_test_reports, project: project) }
+        before do
+          create(:ci_build, name: 'rspec', pipeline: pipeline).tap do |build|
+            create(:ci_job_artifact, :junit, job: build)
+          end
+        end
 
         it 'renders the test report' do
           get_test_report_json
@@ -773,19 +775,22 @@ describe Projects::PipelinesController do
         end
       end
 
-      context 'when pipeline has corrupt test reports' do
-        let(:pipeline) { create(:ci_pipeline, project: project) }
-
+      context 'when pipeline has a corrupt test report artifact' do
         before do
-          job = create(:ci_build, pipeline: pipeline)
-          create(:ci_job_artifact, :junit_with_corrupted_data, job: job, project: project)
-        end
+          create(:ci_build, name: 'rspec', pipeline: pipeline).tap do |build|
+            create(:ci_job_artifact, :junit_with_corrupted_data, job: build)
+          end
 
-        it 'renders the test reports' do
           get_test_report_json
+        end
 
+        it 'renders the test reports' do
           expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response['status']).to eq('error_parsing_report')
+          expect(json_response['test_suites'].count).to eq(1)
+        end
+
+        it 'returns a suite_error on the suite with corrupted XML' do
+          expect(json_response['test_suites'].first['suite_error']).to eq('JUnit XML parsing failed: 1:1: FATAL: Document is empty')
         end
       end
 

spec/factories/ci/builds.rb

@@ -314,6 +314,12 @@ FactoryBot.define do
       end
     end
 
+    trait :broken_test_reports do
+      after(:build) do |build|
+        build.job_artifacts << create(:ci_job_artifact, :junit_with_corrupted_data, job: build)
+      end
+    end
+
     trait :coverage_reports do
       after(:build) do |build|
         build.job_artifacts << create(:ci_job_artifact, :cobertura, job: build)

spec/factories/ci/pipelines.rb

@@ -75,6 +75,14 @@ FactoryBot.define do
         end
       end
 
+      trait :with_broken_test_reports do
+        status { :success }
+
+        after(:build) do |pipeline, _evaluator|
+          pipeline.builds << build(:ci_build, :broken_test_reports, pipeline: pipeline, project: pipeline.project)
+        end
+      end
+
       trait :with_coverage_reports do
         status { :success }
 

spec/frontend/pipelines/test_reports/test_summary_table_spec.js

@@ -37,11 +37,47 @@ describe('Test reports summary table', () => {
 
   describe('when test reports are supplied', () => {
     beforeEach(() => createComponent());
+    const findErrorIcon = () => wrapper.find({ ref: 'suiteErrorIcon' });
 
     it('renders the correct number of rows', () => {
       expect(noSuitesToShow().exists()).toBe(false);
       expect(allSuitesRows().length).toBe(testReports.test_suites.length);
     });
+
+    describe('when there is a suite error', () => {
+      beforeEach(() => {
+        createComponent({
+          test_suites: [
+            {
+              ...testReports.test_suites[0],
+              suite_error: 'Suite Error',
+            },
+          ],
+        });
+      });
+
+      it('renders error icon', () => {
+        expect(findErrorIcon().exists()).toBe(true);
+        expect(findErrorIcon().attributes('title')).toEqual('Suite Error');
+      });
+    });
+
+    describe('when there is not a suite error', () => {
+      beforeEach(() => {
+        createComponent({
+          test_suites: [
+            {
+              ...testReports.test_suites[0],
+              suite_error: null,
+            },
+          ],
+        });
+      });
+
+      it('does not render error icon', () => {
+        expect(findErrorIcon().exists()).toBe(false);
+      });
+    });
   });
 
   describe('when there are no test suites', () => {

spec/javascripts/reports/components/grouped_test_reports_app_spec.js

@@ -4,6 +4,7 @@ import axios from '~/lib/utils/axios_utils';
 import state from '~/reports/store/state';
 import component from '~/reports/components/grouped_test_reports_app.vue';
 import mountComponent from '../../helpers/vue_mount_component_helper';
+import { failedReport } from '../mock_data/mock_data';
 import newFailedTestReports from '../mock_data/new_failures_report.json';
 import newErrorsTestReports from '../mock_data/new_errors_report.json';
 import successTestReports from '../mock_data/no_failures_report.json';
@@ -199,6 +200,26 @@ describe('Grouped Test Reports App', () => {
     });
   });
 
+  describe('with a report that failed to load', () => {
+    beforeEach(() => {
+      mock.onGet('test_results.json').reply(200, failedReport, {});
+      vm = mountComponent(Component, {
+        endpoint: 'test_results.json',
+      });
+    });
+
+    it('renders an error status for the report', done => {
+      setTimeout(() => {
+        const { name } = failedReport.suites[0];
+
+        expect(vm.$el.querySelector('.report-block-list-issue').textContent).toContain(
+          `An error occurred while loading ${name} results`,
+        );
+        done();
+      });
+    });
+  });
+
   describe('with error', () => {
     beforeEach(() => {
       mock.onGet('test_results.json').reply(500, {}, {});

spec/javascripts/reports/mock_data/mock_data.js

-// eslint-disable-next-line import/prefer-default-export
 export const issue = {
   result: 'failure',
   name: 'Test#sum when a is 1 and b is 2 returns summary',
@@ -6,3 +5,20 @@ export const issue = {
   system_output:
     "Failure/Error: is_expected.to eq(3)\n\n  expected: 3\n       got: -1\n\n  (compared using ==)\n./spec/test_spec.rb:12:in `block (4 levels) in \u003ctop (required)\u003e'",
 };
+
+export const failedReport = {
+  summary: { total: 11, resolved: 0, errored: 2, failed: 0 },
+  suites: [
+    {
+      name: 'rspec:pg',
+      status: 'error',
+      summary: { total: 0, resolved: 0, errored: 0, failed: 0 },
+      new_failures: [],
+      resolved_failures: [],
+      existing_failures: [],
+      new_errors: [],
+      resolved_errors: [],
+      existing_errors: [],
+    },
+  ],
+};

spec/lib/gitlab/ci/parsers/test/junit_spec.rb

@@ -215,8 +215,64 @@ describe Gitlab::Ci::Parsers::Test::Junit do
     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)
+      it 'attaches an error to the TestSuite object' do
+        expect { subject }.not_to raise_error
+        expect(test_cases).to be_empty
+      end
+    end
+
+    context 'when data is malformed JUnit XML' do
+      let(:junit) do
+        <<-EOF.strip_heredoc
+          <testsuite>
+            <testcase classname='Calculator' name='sumTest1' time='0.01'></testcase>
+            <testcase classname='Calculator' name='sumTest2' time='0.02'></testcase
+          </testsuite>
+        EOF
+      end
+
+      it 'attaches an error to the TestSuite object' do
+        expect { subject }.not_to raise_error
+        expect(test_suite.suite_error).to eq("JUnit XML parsing failed: 4:1: FATAL: expected '>'")
+      end
+
+      it 'returns 0 tests cases' do
+        subject
+
+        expect(test_cases).to be_empty
+        expect(test_suite.total_count).to eq(0)
+        expect(test_suite.success_count).to eq(0)
+        expect(test_suite.error_count).to eq(0)
+      end
+
+      it 'returns a failure status' do
+        subject
+
+        expect(test_suite.total_status).to eq(Gitlab::Ci::Reports::TestCase::STATUS_ERROR)
+      end
+    end
+
+    context 'when data is not XML' do
+      let(:junit) { double(:random_trash) }
+
+      it 'attaches an error to the TestSuite object' do
+        expect { subject }.not_to raise_error
+        expect(test_suite.suite_error).to eq('JUnit data parsing failed: no implicit conversion of RSpec::Mocks::Double into String')
+      end
+
+      it 'returns 0 tests cases' do
+        subject
+
+        expect(test_cases).to be_empty
+        expect(test_suite.total_count).to eq(0)
+        expect(test_suite.success_count).to eq(0)
+        expect(test_suite.error_count).to eq(0)
+      end
+
+      it 'returns a failure status' do
+        subject
+
+        expect(test_suite.total_status).to eq(Gitlab::Ci::Reports::TestCase::STATUS_ERROR)
       end
     end
 

spec/lib/gitlab/ci/reports/test_reports_spec.rb

@@ -141,6 +141,29 @@ describe Gitlab::Ci::Reports::TestReports do
     end
   end
 
+  describe '#suite_errors' do
+    subject { test_reports.suite_errors }
+
+    context 'when a suite has normal spec errors or failures' do
+      before do
+        test_reports.get_suite('junit').add_test_case(create_test_case_java_success)
+        test_reports.get_suite('junit').add_test_case(create_test_case_java_failed)
+        test_reports.get_suite('junit').add_test_case(create_test_case_java_error)
+      end
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'when there is an error test case' do
+      before do
+        test_reports.get_suite('rspec').add_test_case(create_test_case_rspec_success)
+        test_reports.get_suite('junit').set_suite_error('Existential parsing error')
+      end
+
+      it { is_expected.to eq({ 'junit' => 'Existential parsing error' }) }
+    end
+  end
+
   Gitlab::Ci::Reports::TestCase::STATUS_TYPES.each do |status_type|
     describe "##{status_type}_count" do
       subject { test_reports.public_send("#{status_type}_count") }

spec/lib/gitlab/ci/reports/test_suite_spec.rb

@@ -114,6 +114,31 @@ describe Gitlab::Ci::Reports::TestSuite do
     end
   end
 
+  describe '#set_suite_error' do
+    let(:set_suite_error) { test_suite.set_suite_error('message') }
+
+    context 'when @suite_error is nil' do
+      it 'returns message' do
+        expect(set_suite_error).to eq('message')
+      end
+
+      it 'sets the new message' do
+        set_suite_error
+        expect(test_suite.suite_error).to eq('message')
+      end
+    end
+
+    context 'when a suite_error has already been set' do
+      before do
+        test_suite.set_suite_error('old message')
+      end
+
+      it 'overwrites the existing message' do
+        expect { set_suite_error }.to change(test_suite, :suite_error).from('old message').to('message')
+      end
+    end
+  end
+
   Gitlab::Ci::Reports::TestCase::STATUS_TYPES.each do |status_type|
     describe "##{status_type}" do
       subject { test_suite.public_send("#{status_type}") }

spec/models/ci/build_spec.rb

@@ -3812,8 +3812,13 @@ describe Ci::Build do
           create(:ci_job_artifact, :junit_with_corrupted_data, job: build, project: build.project)
         end
 
-        it 'raises an error' do
-          expect { subject }.to raise_error(Gitlab::Ci::Parsers::Test::Junit::JunitParserError)
+        it 'returns no test data and includes a suite_error message' do
+          expect { subject }.not_to raise_error
+
+          expect(test_reports.get_suite(build.name).total_count).to eq(0)
+          expect(test_reports.get_suite(build.name).success_count).to eq(0)
+          expect(test_reports.get_suite(build.name).failed_count).to eq(0)
+          expect(test_reports.get_suite(build.name).suite_error).to eq('JUnit XML parsing failed: 1:1: FATAL: Document is empty')
         end
       end
     end

spec/serializers/test_suite_entity_spec.rb

@@ -3,27 +3,65 @@
 require 'spec_helper'
 
 describe TestSuiteEntity do
-  let(:pipeline) { create(:ci_pipeline, :with_test_reports) }
-  let(:entity) { described_class.new(pipeline.test_reports.test_suites.each_value.first) }
+  let(:pipeline)   { create(:ci_pipeline, :with_test_reports) }
+  let(:test_suite) { pipeline.test_reports.test_suites.each_value.first }
+  let(:entity)     { described_class.new(test_suite) }
 
   describe '#as_json' do
     subject(:as_json) { entity.as_json }
 
     it 'contains the suite name' do
-      expect(as_json).to include(:name)
+      expect(as_json[:name]).to be_present
     end
 
     it 'contains the total time' do
-      expect(as_json).to include(:total_time)
+      expect(as_json[:total_time]).to be_present
     end
 
     it 'contains the counts' do
-      expect(as_json).to include(:total_count, :success_count, :failed_count, :skipped_count, :error_count)
+      expect(as_json[:total_count]).to eq(4)
+      expect(as_json[:success_count]).to eq(2)
+      expect(as_json[:failed_count]).to eq(2)
+      expect(as_json[:skipped_count]).to eq(0)
+      expect(as_json[:error_count]).to eq(0)
     end
 
     it 'contains the test cases' do
-      expect(as_json).to include(:test_cases)
       expect(as_json[:test_cases].count).to eq(4)
     end
+
+    it 'contains an empty error message' do
+      expect(as_json[:suite_error]).to be_nil
+    end
+
+    context 'with a suite error' do
+      before do
+        test_suite.set_suite_error('a really bad error')
+      end
+
+      it 'contains the suite name' do
+        expect(as_json[:name]).to be_present
+      end
+
+      it 'contains the total time' do
+        expect(as_json[:total_time]).to be_present
+      end
+
+      it 'returns all the counts as 0' do
+        expect(as_json[:total_count]).to eq(0)
+        expect(as_json[:success_count]).to eq(0)
+        expect(as_json[:failed_count]).to eq(0)
+        expect(as_json[:skipped_count]).to eq(0)
+        expect(as_json[:error_count]).to eq(0)
+      end
+
+      it 'returns no test cases' do
+        expect(as_json[:test_cases]).to be_empty
+      end
+
+      it 'returns a suite error' do
+        expect(as_json[:suite_error]).to eq('a really bad error')
+      end
+    end
   end
 end

spec/services/ci/compare_test_reports_service_spec.rb

@@ -38,9 +38,10 @@ describe Ci::CompareTestReportsService do
         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 include('XML parsing failed')
+      it 'returns a parsed TestReports success status and failure on the individual suite' do
+        expect(subject[:status]).to eq(:parsed)
+        expect(subject.dig(:data, 'status')).to eq('success')
+        expect(subject.dig(:data, 'suites', 0, 'status') ).to eq('error')
       end
     end
   end