Fix bugs/edge cases of JUnitParser

changelogs/unreleased/fix-junit-parser.yml

+---
+title: Fix edge cases of JUnitParser
+merge_request: 21469
+author:
+type: fixed

doc/ci/junit_test_reports.md

@@ -139,3 +139,11 @@ java:
         - target/surefire-reports/TEST-*.xml
         - target/failsafe-reports/TEST-*.xml
 ```
+
+## Limitations
+
+Currently, the following tools might not work because their XML formats are unsupported in GitLab.
+
+|Case|Tool|Issue|
+|---|---|---|
+|`<testcase>` does not have `classname` attribute|ESlint, sass-lint|https://gitlab.com/gitlab-org/gitlab-ce/issues/50964|

lib/gitlab/ci/parsers/junit.rb

@@ -2,18 +2,14 @@ module Gitlab
   module Ci
     module Parsers
       class Junit
-        attr_reader :data
-
         JunitParserError = Class.new(StandardError)
 
         def parse!(xml_data, test_suite)
-          @data = Hash.from_xml(xml_data)
+          root = Hash.from_xml(xml_data)
 
-          each_suite do |testcases|
-            testcases.each do |testcase|
-              test_case = create_test_case(testcase)
-              test_suite.add_test_case(test_case)
-            end
+          all_cases(root) do |test_case|
+            test_case = create_test_case(test_case)
+            test_suite.add_test_case(test_case)
           end
         rescue REXML::ParseException => e
           raise JunitParserError, "XML parsing failed: #{e.message}"
@@ -23,26 +19,27 @@ module Gitlab
 
         private
 
-        def each_suite
-          testsuites.each do |testsuite|
-            yield testcases(testsuite)
-          end
-        end
+        def all_cases(root, parent = nil, &blk)
+          return unless root.present?
 
-        def testsuites
-          if data['testsuites']
-            data['testsuites']['testsuite']
-          else
-            [data['testsuite']]
+          [root].flatten.compact.map do |node|
+            next unless node.is_a?(Hash)
+
+            # we allow only one top-level 'testsuites'
+            all_cases(node['testsuites'], root, &blk) unless parent
+
+            # we require at least one level of testsuites or testsuite
+            each_case(node['testcase'], &blk) if parent
+
+            # we allow multiple nested 'testsuite' (eg. PHPUnit)
+            all_cases(node['testsuite'], root, &blk)
           end
         end
 
-        def testcases(testsuite)
-          if testsuite['testcase'].is_a?(Array)
-            testsuite['testcase']
-          else
-            [testsuite['testcase']]
-          end
+        def each_case(testcase, &blk)
+          return unless testcase.present?
+
+          [testcase].flatten.compact.map(&blk)
         end
 
         def create_test_case(data)

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

-require 'spec_helper'
+require 'fast_spec_helper'
 
 describe Gitlab::Ci::Parsers::Junit do
   describe '#parse!' do
@@ -8,21 +8,35 @@ describe Gitlab::Ci::Parsers::Junit do
     let(:test_cases) { flattened_test_cases(test_suite) }
 
     context 'when data is JUnit style XML' do
-      context 'when there are no test cases' do
+      context 'when there are no <testcases> in <testsuite>' do
         let(:junit) do
           <<-EOF.strip_heredoc
             <testsuite></testsuite>
           EOF
         end
 
-        it 'raises an error and does not add any test cases' do
-          expect { subject }.to raise_error(described_class::JunitParserError)
+        it 'ignores the case' do
+          expect { subject }.not_to raise_error
+
+          expect(test_cases.count).to eq(0)
+        end
+      end
+
+      context 'when there are no <testcases> in <testsuites>' do
+        let(:junit) do
+          <<-EOF.strip_heredoc
+            <testsuites><testsuite /></testsuites>
+          EOF
+        end
+
+        it 'ignores the case' do
+          expect { subject }.not_to raise_error
 
           expect(test_cases.count).to eq(0)
         end
       end
 
-      context 'when there is a test case' do
+      context 'when there is only one <testcase> in <testsuite>' do
         let(:junit) do
           <<-EOF.strip_heredoc
             <testsuite>
@@ -40,6 +54,46 @@ describe Gitlab::Ci::Parsers::Junit do
         end
       end
 
+      context 'when there is only one <testsuite> in <testsuites>' do
+        let(:junit) do
+          <<-EOF.strip_heredoc
+            <testsuites>
+              <testsuite>
+                <testcase classname='Calculator' name='sumTest1' time='0.01'></testcase>
+              </testsuite>
+            </testsuites>
+          EOF
+        end
+
+        it 'parses XML and adds a test case to a suite' do
+          expect { subject }.not_to raise_error
+
+          expect(test_cases[0].classname).to eq('Calculator')
+          expect(test_cases[0].name).to eq('sumTest1')
+          expect(test_cases[0].execution_time).to eq(0.01)
+        end
+      end
+
+      context 'PHPUnit' do
+        let(:junit) do
+          <<-EOF.strip_heredoc
+          <testsuites>
+            <testsuite name="Project Test Suite" tests="1" assertions="1" failures="0" errors="0" time="1.376748">
+              <testsuite name="XXX\\FrontEnd\\WebBundle\\Tests\\Controller\\LogControllerTest" file="/Users/mcfedr/projects/xxx/server/tests/XXX/FrontEnd/WebBundle/Tests/Controller/LogControllerTest.php" tests="1" assertions="1" failures="0" errors="0" time="1.376748">
+                <testcase name="testIndexAction" class="XXX\\FrontEnd\\WebBundle\\Tests\\Controller\\LogControllerTest" file="/Users/mcfedr/projects/xxx/server/tests/XXX/FrontEnd/WebBundle/Tests/Controller/LogControllerTest.php" line="9" assertions="1" time="1.376748"/>
+              </testsuite>
+            </testsuite>
+          </testsuites>
+          EOF
+        end
+
+        it 'parses XML and adds a test case to a suite' do
+          expect { subject }.not_to raise_error
+
+          expect(test_cases.count).to eq(1)
+        end
+      end
+
       context 'when there are two test cases' do
         let(:junit) do
           <<-EOF.strip_heredoc