Merge branch 'eb-suitename-in-testcase-key' into 'master'

Include testsuite name in test case key

See merge request gitlab-org/gitlab!44473

app/services/ci/build_report_result_service.rb

@@ -47,7 +47,7 @@ module Ci
     def test_case_hashes(build, test_suite)
       [].tap do |hashes|
         test_suite.each_test_case do |test_case|
-          key = "#{build.project_id}-#{test_suite.name}-#{test_case.key}"
+          key = "#{build.project_id}-#{test_case.key}"
           hashes << Digest::SHA256.hexdigest(key)
         end
       end

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

@@ -12,7 +12,7 @@ module Gitlab
             root = Hash.from_xml(xml_data)
 
             all_cases(root) do |test_case|
-              test_case = create_test_case(test_case, args)
+              test_case = create_test_case(test_case, test_suite, args)
               test_suite.add_test_case(test_case)
             end
           rescue Nokogiri::XML::SyntaxError => e
@@ -33,20 +33,24 @@ module Gitlab
               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
+              each_case(node['testcase'], node['name'], &blk) if parent
 
               # we allow multiple nested 'testsuite' (eg. PHPUnit)
               all_cases(node['testsuite'], root, &blk)
             end
           end
 
-          def each_case(testcase, &blk)
+          def each_case(testcase, testsuite_name, &blk)
             return unless testcase.present?
 
-            [testcase].flatten.compact.map(&blk)
+            [testcase].flatten.compact.each do |tc|
+              tc['suite_name'] = testsuite_name
+
+              yield(tc)
+            end
           end
 
-          def create_test_case(data, args)
+          def create_test_case(data, test_suite, args)
             if data.key?('failure')
               status = ::Gitlab::Ci::Reports::TestCase::STATUS_FAILED
               system_output = data['failure']
@@ -63,6 +67,7 @@ module Gitlab
             end
 
             ::Gitlab::Ci::Reports::TestCase.new(
+              suite_name: data['suite_name'] || test_suite.name,
               classname: data['classname'],
               name: data['name'],
               file: data['file'],
@@ -74,6 +79,10 @@ module Gitlab
             )
           end
 
+          def suite_name(parent, test_suite)
+            parent.dig('testsuite', 'name') || test_suite.name
+          end
+
           def attachment_path(data)
             return unless data
 

lib/gitlab/ci/reports/test_case.rb

@@ -10,9 +10,10 @@ module Gitlab
         STATUS_ERROR = 'error'
         STATUS_TYPES = [STATUS_ERROR, STATUS_FAILED, STATUS_SUCCESS, STATUS_SKIPPED].freeze
 
-        attr_reader :name, :classname, :execution_time, :status, :file, :system_output, :stack_trace, :key, :attachment, :job
+        attr_reader :suite_name, :name, :classname, :execution_time, :status, :file, :system_output, :stack_trace, :key, :attachment, :job
 
         def initialize(params)
+          @suite_name = params.fetch(:suite_name)
           @name = params.fetch(:name)
           @classname = params.fetch(:classname)
           @file = params.fetch(:file, nil)
@@ -23,7 +24,7 @@ module Gitlab
           @attachment = params.fetch(:attachment, nil)
           @job = params.fetch(:job, nil)
 
-          @key = hash_key("#{classname}_#{name}")
+          @key = hash_key("#{suite_name}_#{classname}_#{name}")
         end
 
         def has_attachment?

spec/factories/ci/test_case.rb

@@ -2,6 +2,7 @@
 
 FactoryBot.define do
   factory :test_case, class: 'Gitlab::Ci::Reports::TestCase' do
+    suite_name { "rspec" }
     name { "test-1" }
     classname { "trace" }
     file { "spec/trace_spec.rb" }
@@ -25,6 +26,7 @@ FactoryBot.define do
 
     initialize_with do
       new(
+        suite_name: suite_name,
         name: name,
         classname: classname,
         file: file,

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

@@ -43,7 +43,7 @@ RSpec.describe Gitlab::Ci::Parsers::Test::Junit do
         let(:junit) do
           <<-EOF.strip_heredoc
             <testsuites>
-              <testsuite>
+              <testsuite name='Math'>
                 <testcase classname='Calculator' name='sumTest1' time='0.01'></testcase>
               </testsuite>
             </testsuites>
@@ -53,6 +53,7 @@ RSpec.describe Gitlab::Ci::Parsers::Test::Junit do
         it 'parses XML and adds a test case to a suite' do
           expect { subject }.not_to raise_error
 
+          expect(test_cases[0].suite_name).to eq('Math')
           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)
@@ -62,7 +63,7 @@ RSpec.describe Gitlab::Ci::Parsers::Test::Junit do
       context 'when there is <testcase>' do
         let(:junit) do
           <<-EOF.strip_heredoc
-              <testsuite>
+              <testsuite name='Math'>
                 <testcase classname='Calculator' name='sumTest1' time='0.01'>
                   #{testcase_content}
                 </testcase>
@@ -79,6 +80,7 @@ RSpec.describe Gitlab::Ci::Parsers::Test::Junit do
         shared_examples_for '<testcase> XML parser' do |status, output|
           it 'parses XML and adds a test case to the suite' do
             aggregate_failures do
+              expect(test_case.suite_name).to eq('Math')
               expect(test_case.classname).to eq('Calculator')
               expect(test_case.name).to eq('sumTest1')
               expect(test_case.execution_time).to eq(0.01)
@@ -152,13 +154,15 @@ RSpec.describe Gitlab::Ci::Parsers::Test::Junit do
           expect { subject }.not_to raise_error
 
           expect(test_cases.count).to eq(1)
+          expect(test_cases.first.suite_name).to eq("XXX\\FrontEnd\\WebBundle\\Tests\\Controller\\LogControllerTest")
+          expect(test_cases.first.name).to eq("testIndexAction")
         end
       end
 
       context 'when there are two test cases' do
         let(:junit) do
           <<-EOF.strip_heredoc
-            <testsuite>
+            <testsuite name='Math'>
               <testcase classname='Calculator' name='sumTest1' time='0.01'></testcase>
               <testcase classname='Calculator' name='sumTest2' time='0.02'></testcase>
             </testsuite>
@@ -168,9 +172,11 @@ RSpec.describe Gitlab::Ci::Parsers::Test::Junit do
         it 'parses XML and adds test cases to a suite' do
           expect { subject }.not_to raise_error
 
+          expect(test_cases[0].suite_name).to eq('Math')
           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)
+          expect(test_cases[1].suite_name).to eq('Math')
           expect(test_cases[1].classname).to eq('Calculator')
           expect(test_cases[1].name).to eq('sumTest2')
           expect(test_cases[1].execution_time).to eq(0.02)
@@ -181,7 +187,7 @@ RSpec.describe Gitlab::Ci::Parsers::Test::Junit do
         let(:junit) do
           <<-EOF.strip_heredoc
             <testsuites>
-              <testsuite>
+              <testsuite name='Math'>
                 <testcase classname='Calculator' name='sumTest1' time='0.01'></testcase>
                 <testcase classname='Calculator' name='sumTest2' time='0.02'></testcase>
               </testsuite>
@@ -196,18 +202,32 @@ RSpec.describe Gitlab::Ci::Parsers::Test::Junit do
         it 'parses XML and adds test cases 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)
-          expect(test_cases[1].classname).to eq('Calculator')
-          expect(test_cases[1].name).to eq('sumTest2')
-          expect(test_cases[1].execution_time).to eq(0.02)
-          expect(test_cases[2].classname).to eq('Statemachine')
-          expect(test_cases[2].name).to eq('happy path')
-          expect(test_cases[2].execution_time).to eq(100)
-          expect(test_cases[3].classname).to eq('Statemachine')
-          expect(test_cases[3].name).to eq('unhappy path')
-          expect(test_cases[3].execution_time).to eq(200)
+          expect(test_cases).to contain_exactly(
+            have_attributes(
+              suite_name: 'Math',
+              classname: 'Calculator',
+              name: 'sumTest1',
+              execution_time: 0.01
+            ),
+            have_attributes(
+              suite_name: 'Math',
+              classname: 'Calculator',
+              name: 'sumTest2',
+              execution_time: 0.02
+            ),
+            have_attributes(
+              suite_name: test_suite.name, # Defaults to test suite instance's name
+              classname: 'Statemachine',
+              name: 'happy path',
+              execution_time: 100
+            ),
+            have_attributes(
+              suite_name: test_suite.name, # Defaults to test suite instance's name
+              classname: 'Statemachine',
+              name: 'unhappy path',
+              execution_time: 200
+            )
+          )
         end
       end
     end

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

@@ -6,39 +6,26 @@ RSpec.describe Gitlab::Ci::Reports::TestCase do
   describe '#initialize' do
     let(:test_case) { described_class.new(params) }
 
-    context 'when both classname and name are given' do
-      context 'when test case is passed' do
+    context 'when required params are given' do
       let(:job) { build(:ci_build) }
       let(:params) { attributes_for(:test_case).merge!(job: job) }
 
-        it 'initializes an instance' do
+      it 'initializes an instance', :aggregate_failures do
         expect { test_case }.not_to raise_error
 
-          expect(test_case.name).to eq('test-1')
-          expect(test_case.classname).to eq('trace')
-          expect(test_case.file).to eq('spec/trace_spec.rb')
-          expect(test_case.execution_time).to eq(1.23)
-          expect(test_case.status).to eq(described_class::STATUS_SUCCESS)
-          expect(test_case.system_output).to be_nil
-          expect(test_case.job).to be_present
-        end
-      end
+        expect(test_case).to have_attributes(
+          suite_name: params[:suite_name],
+          name: params[:name],
+          classname: params[:classname],
+          file: params[:file],
+          execution_time: params[:execution_time],
+          status: params[:status],
+          system_output: params[:system_output],
+          job: params[:job]
+        )
 
-      context 'when test case is failed' do
-        let(:job) { build(:ci_build) }
-        let(:params) { attributes_for(:test_case, :failed).merge!(job: job) }
-
-        it 'initializes an instance' do
-          expect { test_case }.not_to raise_error
-
-          expect(test_case.name).to eq('test-1')
-          expect(test_case.classname).to eq('trace')
-          expect(test_case.file).to eq('spec/trace_spec.rb')
-          expect(test_case.execution_time).to eq(1.23)
-          expect(test_case.status).to eq(described_class::STATUS_FAILED)
-          expect(test_case.system_output)
-            .to eq('Failure/Error: is_expected.to eq(300) expected: 300 got: -100')
-        end
+        key = "#{test_case.suite_name}_#{test_case.classname}_#{test_case.name}"
+        expect(test_case.key).to eq(Digest::SHA256.hexdigest(key))
       end
     end
 
@@ -53,6 +40,10 @@ RSpec.describe Gitlab::Ci::Reports::TestCase do
       end
     end
 
+    context 'when suite_name is missing' do
+      it_behaves_like 'param is missing', :suite_name
+    end
+
     context 'when classname is missing' do
       it_behaves_like 'param is missing', :classname
     end

spec/support/test_reports/test_reports_helper.rb

@@ -3,6 +3,7 @@
 module TestReportsHelper
   def create_test_case_rspec_success(name = 'test_spec')
     Gitlab::Ci::Reports::TestCase.new(
+      suite_name: 'rspec',
       name: 'Test#sum when a is 1 and b is 3 returns summary',
       classname: "spec.#{name}",
       file: './spec/test_spec.rb',
@@ -12,6 +13,7 @@ module TestReportsHelper
 
   def create_test_case_rspec_failed(name = 'test_spec', execution_time = 2.22)
     Gitlab::Ci::Reports::TestCase.new(
+      suite_name: 'rspec',
       name: 'Test#sum when a is 1 and b is 3 returns summary',
       classname: "spec.#{name}",
       file: './spec/test_spec.rb',
@@ -22,6 +24,7 @@ module TestReportsHelper
 
   def create_test_case_rspec_skipped(name = 'test_spec')
     Gitlab::Ci::Reports::TestCase.new(
+      suite_name: 'rspec',
       name: 'Test#sum when a is 3 and b is 3 returns summary',
       classname: "spec.#{name}",
       file: './spec/test_spec.rb',
@@ -31,6 +34,7 @@ module TestReportsHelper
 
   def create_test_case_rspec_error(name = 'test_spec')
     Gitlab::Ci::Reports::TestCase.new(
+      suite_name: 'rspec',
       name: 'Test#sum when a is 4 and b is 4 returns summary',
       classname: "spec.#{name}",
       file: './spec/test_spec.rb',
@@ -52,6 +56,7 @@ module TestReportsHelper
 
   def create_test_case_java_success(name = 'addTest')
     Gitlab::Ci::Reports::TestCase.new(
+      suite_name: 'java',
       name: name,
       classname: 'CalculatorTest',
       execution_time: 5.55,
@@ -60,6 +65,7 @@ module TestReportsHelper
 
   def create_test_case_java_failed(name = 'addTest')
     Gitlab::Ci::Reports::TestCase.new(
+      suite_name: 'java',
       name: name,
       classname: 'CalculatorTest',
       execution_time: 6.66,
@@ -69,6 +75,7 @@ module TestReportsHelper
 
   def create_test_case_java_skipped(name = 'addTest')
     Gitlab::Ci::Reports::TestCase.new(
+      suite_name: 'java',
       name: name,
       classname: 'CalculatorTest',
       execution_time: 7.77,
@@ -77,6 +84,7 @@ module TestReportsHelper
 
   def create_test_case_java_error(name = 'addTest')
     Gitlab::Ci::Reports::TestCase.new(
+      suite_name: 'java',
       name: name,
       classname: 'CalculatorTest',
       execution_time: 8.88,