Include testsuite name in test case key

We need to include the testsuite name (which also fallbacks to the
build's group name) in the test case key to make it more reliably
unique across the project.

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,