Merge branch 'eb-test-failure-history-mvc' into 'master'

Store test failure data when build finishes

See merge request gitlab-org/gitlab!45027

app/models/ci/test_case.rb

+# frozen_string_literal: true
+
+module Ci
+  class TestCase < ApplicationRecord
+    extend Gitlab::Ci::Model
+
+    validates :project, :key_hash, presence: true
+
+    has_many :test_case_failures, class_name: 'Ci::TestCaseFailure'
+
+    belongs_to :project
+
+    scope :by_project_and_keys, -> (project, keys) { where(project_id: project.id, key_hash: keys) }
+
+    class << self
+      def find_or_create_by_batch(project, test_case_keys)
+        # Insert records first. Existing ones will be skipped.
+        insert_all(test_case_attrs(project, test_case_keys))
+
+        # Find all matching records now that we are sure they all are persisted.
+        by_project_and_keys(project, test_case_keys)
+      end
+
+      private
+
+      def test_case_attrs(project, test_case_keys)
+        # NOTE: Rails 6.1 will add support for insert_all on relation so that
+        # we will be able to do project.test_cases.insert_all.
+        test_case_keys.map do |hashed_key|
+          { project_id: project.id, key_hash: hashed_key }
+        end
+      end
+    end
+  end
+end

app/models/ci/test_case_failure.rb

+# frozen_string_literal: true
+
+module Ci
+  class TestCaseFailure < ApplicationRecord
+    extend Gitlab::Ci::Model
+
+    validates :test_case, :build, :failed_at, presence: true
+
+    belongs_to :test_case, class_name: "Ci::TestCase", foreign_key: :test_case_id
+    belongs_to :build, class_name: "Ci::Build", foreign_key: :build_id
+  end
+end

app/services/ci/test_cases_service.rb

+# frozen_string_literal: true
+
+module Ci
+  class TestCasesService
+    MAX_TRACKABLE_FAILURES = 200
+
+    def execute(build)
+      return unless Feature.enabled?(:test_failure_history, build.project)
+      return unless build.has_test_reports?
+      return unless build.project.default_branch_or_master == build.ref
+
+      test_suite = generate_test_suite_report(build)
+
+      track_failures(build, test_suite)
+    end
+
+    private
+
+    def generate_test_suite_report(build)
+      build.collect_test_reports!(Gitlab::Ci::Reports::TestReports.new)
+    end
+
+    def track_failures(build, test_suite)
+      return if test_suite.failed_count > MAX_TRACKABLE_FAILURES
+
+      test_suite.failed.keys.each_slice(100) do |keys|
+        Ci::TestCase.transaction do
+          test_cases = Ci::TestCase.find_or_create_by_batch(build.project, keys)
+          Ci::TestCaseFailure.insert_all(test_case_failures(test_cases, build))
+        end
+      end
+    end
+
+    def test_case_failures(test_cases, build)
+      test_cases.map do |test_case|
+        {
+          test_case_id: test_case.id,
+          build_id: build.id,
+          failed_at: build.finished_at
+        }
+      end
+    end
+  end
+end

app/workers/build_finished_worker.rb

@@ -33,6 +33,11 @@ class BuildFinishedWorker # rubocop:disable Scalability/IdempotentWorker
     BuildCoverageWorker.new.perform(build.id)
     Ci::BuildReportResultWorker.new.perform(build.id)
 
+    # TODO: As per https://gitlab.com/groups/gitlab-com/gl-infra/-/epics/194, it may be
+    # best to avoid creating more workers that we have no intention of calling async.
+    # Change the previous worker calls on top to also just call the service directly.
+    Ci::TestCasesService.new.execute(build)
+
     # We execute these async as these are independent operations.
     BuildHooksWorker.perform_async(build.id)
     ExpirePipelineCacheWorker.perform_async(build.pipeline_id) if build.pipeline.cacheable?

changelogs/unreleased/eb-test-failure-history-mvc.yml

+---
+title: Store test failure data when build finishes
+merge_request: 45027
+author:
+type: added

config/feature_flags/development/test_failure_history.yml

+---
+name: test_failure_history
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/45027
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/268249
+type: development
+group: group::testing
+default_enabled: false

db/migrate/20201012134230_create_ci_test_cases.rb

+# frozen_string_literal: true
+
+class CreateCiTestCases < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    unless table_exists?(:ci_test_cases)
+      create_table :ci_test_cases do |t|
+        t.bigint :project_id, null: false
+        t.text :key_hash, null: false
+
+        t.index [:project_id, :key_hash], unique: true
+        # NOTE: FK for projects will be added on a separate migration as per guidelines
+      end
+    end
+
+    add_text_limit :ci_test_cases, :key_hash, 64
+  end
+
+  def down
+    drop_table :ci_test_cases
+  end
+end

db/migrate/20201012135330_create_ci_test_case_failures.rb

+# frozen_string_literal: true
+
+class CreateCiTestCaseFailures < ActiveRecord::Migration[6.0]
+  DOWNTIME = false
+
+  def up
+    create_table :ci_test_case_failures do |t|
+      t.datetime_with_timezone :failed_at
+      t.bigint :test_case_id, null: false
+      t.bigint :build_id, null: false
+
+      t.index [:test_case_id, :failed_at, :build_id], name: 'index_test_case_failures_unique_columns', unique: true, order: { failed_at: :desc }
+      t.index :build_id
+      t.foreign_key :ci_test_cases, column: :test_case_id, on_delete: :cascade
+      # NOTE: FK for ci_builds will be added on a separate migration as per guidelines
+    end
+  end
+
+  def down
+    drop_table :ci_test_case_failures
+  end
+end

db/migrate/20201012140110_add_projects_fk_to_ci_test_cases.rb

+# frozen_string_literal: true
+
+class AddProjectsFkToCiTestCases < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :ci_test_cases, :projects, column: :project_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :ci_test_cases, column: :project_id
+    end
+  end
+end

db/migrate/20201012140452_add_ci_builds_fk_to_ci_test_case_failures.rb

+# frozen_string_literal: true
+
+class AddCiBuildsFkToCiTestCaseFailures < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_foreign_key :ci_test_case_failures, :ci_builds, column: :build_id, on_delete: :cascade
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :ci_test_case_failures, column: :build_id
+    end
+  end
+end

db/schema_migrations/20201012134230

+1673018885366e92eb47f5fc705ea8251c2db49b5c14b788e84b10d8db91af48
\ No newline at end of file

db/schema_migrations/20201012135330

+18ccd2059d9a19a51ea0162c46a1293e280759daffa54ba58ba5e431ee7aba93
\ No newline at end of file

db/schema_migrations/20201012140110

+e266655483655e1ecbb4f65594ef5b985c3f0449231755f589f3e293e28c9f6b
\ No newline at end of file

db/schema_migrations/20201012140452

+d62928276708c26656070f803ea6271be74a1fe9802877258d4a8cf19df32d09
\ No newline at end of file

db/structure.sql

@@ -10656,6 +10656,38 @@ CREATE SEQUENCE ci_subscriptions_projects_id_seq
 
 ALTER SEQUENCE ci_subscriptions_projects_id_seq OWNED BY ci_subscriptions_projects.id;
 
+CREATE TABLE ci_test_case_failures (
+    id bigint NOT NULL,
+    failed_at timestamp with time zone,
+    test_case_id bigint NOT NULL,
+    build_id bigint NOT NULL
+);
+
+CREATE SEQUENCE ci_test_case_failures_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE ci_test_case_failures_id_seq OWNED BY ci_test_case_failures.id;
+
+CREATE TABLE ci_test_cases (
+    id bigint NOT NULL,
+    project_id bigint NOT NULL,
+    key_hash text NOT NULL,
+    CONSTRAINT check_dd3c5d1c15 CHECK ((char_length(key_hash) <= 64))
+);
+
+CREATE SEQUENCE ci_test_cases_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE ci_test_cases_id_seq OWNED BY ci_test_cases.id;
+
 CREATE TABLE ci_trigger_requests (
     id integer NOT NULL,
     trigger_id integer NOT NULL,
@@ -17535,6 +17567,10 @@ ALTER TABLE ONLY ci_stages ALTER COLUMN id SET DEFAULT nextval('ci_stages_id_seq
 
 ALTER TABLE ONLY ci_subscriptions_projects ALTER COLUMN id SET DEFAULT nextval('ci_subscriptions_projects_id_seq'::regclass);
 
+ALTER TABLE ONLY ci_test_case_failures ALTER COLUMN id SET DEFAULT nextval('ci_test_case_failures_id_seq'::regclass);
+
+ALTER TABLE ONLY ci_test_cases ALTER COLUMN id SET DEFAULT nextval('ci_test_cases_id_seq'::regclass);
+
 ALTER TABLE ONLY ci_trigger_requests ALTER COLUMN id SET DEFAULT nextval('ci_trigger_requests_id_seq'::regclass);
 
 ALTER TABLE ONLY ci_triggers ALTER COLUMN id SET DEFAULT nextval('ci_triggers_id_seq'::regclass);
@@ -18575,6 +18611,12 @@ ALTER TABLE ONLY ci_stages
 ALTER TABLE ONLY ci_subscriptions_projects
     ADD CONSTRAINT ci_subscriptions_projects_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY ci_test_case_failures
+    ADD CONSTRAINT ci_test_case_failures_pkey PRIMARY KEY (id);
+
+ALTER TABLE ONLY ci_test_cases
+    ADD CONSTRAINT ci_test_cases_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY ci_trigger_requests
     ADD CONSTRAINT ci_trigger_requests_pkey PRIMARY KEY (id);
 
@@ -20239,6 +20281,10 @@ CREATE INDEX index_ci_subscriptions_projects_on_upstream_project_id ON ci_subscr
 
 CREATE UNIQUE INDEX index_ci_subscriptions_projects_unique_subscription ON ci_subscriptions_projects USING btree (downstream_project_id, upstream_project_id);
 
+CREATE INDEX index_ci_test_case_failures_on_build_id ON ci_test_case_failures USING btree (build_id);
+
+CREATE UNIQUE INDEX index_ci_test_cases_on_project_id_and_key_hash ON ci_test_cases USING btree (project_id, key_hash);
+
 CREATE INDEX index_ci_trigger_requests_on_commit_id ON ci_trigger_requests USING btree (commit_id);
 
 CREATE INDEX index_ci_trigger_requests_on_trigger_id_and_id ON ci_trigger_requests USING btree (trigger_id, id DESC);
@@ -21741,6 +21787,8 @@ CREATE UNIQUE INDEX index_terraform_states_on_project_id_and_name ON terraform_s
 
 CREATE UNIQUE INDEX index_terraform_states_on_uuid ON terraform_states USING btree (uuid);
 
+CREATE UNIQUE INDEX index_test_case_failures_unique_columns ON ci_test_case_failures USING btree (test_case_id, failed_at DESC, build_id);
+
 CREATE INDEX index_timelogs_on_issue_id ON timelogs USING btree (issue_id);
 
 CREATE INDEX index_timelogs_on_merge_request_id ON timelogs USING btree (merge_request_id);
@@ -22329,6 +22377,9 @@ ALTER TABLE ONLY clusters_applications_runners
 ALTER TABLE ONLY design_management_designs_versions
     ADD CONSTRAINT fk_03c671965c FOREIGN KEY (design_id) REFERENCES design_management_designs(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY ci_test_cases
+    ADD CONSTRAINT fk_0526c30ded FOREIGN KEY (project_id) REFERENCES projects(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY issues
     ADD CONSTRAINT fk_05f1e72feb FOREIGN KEY (author_id) REFERENCES users(id) ON DELETE SET NULL;
 
@@ -22842,6 +22893,9 @@ ALTER TABLE ONLY ci_sources_pipelines
 ALTER TABLE ONLY geo_event_log
     ADD CONSTRAINT fk_d5af95fcd9 FOREIGN KEY (lfs_object_deleted_event_id) REFERENCES geo_lfs_object_deleted_events(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY ci_test_case_failures
+    ADD CONSTRAINT fk_d69404d827 FOREIGN KEY (build_id) REFERENCES ci_builds(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY lists
     ADD CONSTRAINT fk_d6cf4279f7 FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE;
 
@@ -24135,6 +24189,9 @@ ALTER TABLE ONLY merge_request_blocks
 ALTER TABLE ONLY protected_branch_unprotect_access_levels
     ADD CONSTRAINT fk_rails_e9eb8dc025 FOREIGN KEY (protected_branch_id) REFERENCES protected_branches(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY ci_test_case_failures
+    ADD CONSTRAINT fk_rails_eab6349715 FOREIGN KEY (test_case_id) REFERENCES ci_test_cases(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY alert_management_alert_user_mentions
     ADD CONSTRAINT fk_rails_eb2de0cdef FOREIGN KEY (note_id) REFERENCES notes(id) ON DELETE CASCADE;
 

spec/factories/ci/builds.rb

@@ -332,6 +332,12 @@ FactoryBot.define do
       end
     end
 
+    trait :test_reports_with_duplicate_failed_test_names do
+      after(:build) do |build|
+        build.job_artifacts << create(:ci_job_artifact, :junit_with_duplicate_failed_test_names, job: build)
+      end
+    end
+
     trait :accessibility_reports do
       after(:build) do |build|
         build.job_artifacts << create(:ci_job_artifact, :accessibility, job: build)

spec/factories/ci/job_artifacts.rb

@@ -109,6 +109,16 @@ FactoryBot.define do
       end
     end
 
+    trait :junit_with_duplicate_failed_test_names do
+      file_type { :junit }
+      file_format { :gzip }
+
+      after(:build) do |artifact, evaluator|
+        artifact.file = fixture_file_upload(
+          Rails.root.join('spec/fixtures/junit/junit_with_duplicate_failed_test_names.xml.gz'), 'application/x-gzip')
+      end
+    end
+
     trait :junit_with_ant do
       file_type { :junit }
       file_format { :gzip }

spec/factories/ci/reports/test_case.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :report_test_case, class: 'Gitlab::Ci::Reports::TestCase' do
+    suite_name { "rspec" }
+    name { "test-1" }
+    classname { "trace" }
+    file { "spec/trace_spec.rb" }
+    execution_time { 1.23 }
+    status { Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS }
+    system_output { nil }
+    attachment { nil }
+    association :job, factory: :ci_build
+
+    trait :failed do
+      status { Gitlab::Ci::Reports::TestCase::STATUS_FAILED }
+      system_output { "Failure/Error: is_expected.to eq(300) expected: 300 got: -100" }
+    end
+
+    trait :failed_with_attachment do
+      status { Gitlab::Ci::Reports::TestCase::STATUS_FAILED }
+      attachment { "some/path.png" }
+    end
+
+    skip_create
+
+    initialize_with do
+      new(
+        suite_name: suite_name,
+        name: name,
+        classname: classname,
+        file: file,
+        execution_time: execution_time,
+        status: status,
+        system_output: system_output,
+        attachment: attachment,
+        job: job
+      )
+    end
+  end
+end

spec/factories/ci/test_case.rb

 # frozen_string_literal: true
 
 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" }
-    execution_time { 1.23 }
-    status { Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS }
-    system_output { nil }
-    attachment { nil }
-    association :job, factory: :ci_build
-
-    trait :failed do
-      status { Gitlab::Ci::Reports::TestCase::STATUS_FAILED }
-      system_output { "Failure/Error: is_expected.to eq(300) expected: 300 got: -100" }
-    end
-
-    trait :failed_with_attachment do
-      status { Gitlab::Ci::Reports::TestCase::STATUS_FAILED }
-      attachment { "some/path.png" }
-    end
-
-    skip_create
-
-    initialize_with do
-      new(
-        suite_name: suite_name,
-        name: name,
-        classname: classname,
-        file: file,
-        execution_time: execution_time,
-        status: status,
-        system_output: system_output,
-        attachment: attachment,
-        job: job
-      )
-    end
+  factory :ci_test_case, class: 'Ci::TestCase' do
+    project
+    key_hash { Digest::SHA256.hexdigest(SecureRandom.hex) }
   end
 end

spec/factories/ci/test_case_failure.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :ci_test_case_failure, class: 'Ci::TestCaseFailure' do
+    build factory: :ci_build
+    test_case factory: :ci_test_case
+    failed_at { Time.current }
+  end
+end

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

@@ -8,7 +8,7 @@ RSpec.describe Gitlab::Ci::Reports::TestCase do
 
     context 'when required params are given' do
       let(:job) { build(:ci_build) }
-      let(:params) { attributes_for(:test_case).merge!(job: job) }
+      let(:params) { attributes_for(:report_test_case).merge!(job: job) }
 
       it 'initializes an instance', :aggregate_failures do
         expect { test_case }.not_to raise_error
@@ -31,7 +31,7 @@ RSpec.describe Gitlab::Ci::Reports::TestCase do
 
     shared_examples 'param is missing' do |param|
       let(:job) { build(:ci_build) }
-      let(:params) { attributes_for(:test_case).merge!(job: job) }
+      let(:params) { attributes_for(:report_test_case).merge!(job: job) }
 
       it 'raises an error' do
         params.delete(param)
@@ -55,7 +55,7 @@ RSpec.describe Gitlab::Ci::Reports::TestCase do
     context 'when attachment is present' do
       let_it_be(:job) { create(:ci_build) }
 
-      let(:attachment_test_case) { build(:test_case, :failed_with_attachment, job: job) }
+      let(:attachment_test_case) { build(:report_test_case, :failed_with_attachment, job: job) }
 
       it "initializes the attachment if present" do
         expect(attachment_test_case.attachment).to eq("some/path.png")
@@ -71,7 +71,7 @@ RSpec.describe Gitlab::Ci::Reports::TestCase do
     end
 
     context 'when attachment is missing' do
-      let(:test_case) { build(:test_case) }
+      let(:test_case) { build(:report_test_case) }
 
       it '#has_attachment?' do
         expect(test_case.has_attachment?).to be_falsy

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

@@ -110,7 +110,7 @@ RSpec.describe Gitlab::Ci::Reports::TestReports do
   end
 
   describe '#with_attachment' do
-    let(:test_case) { build(:test_case, :failed) }
+    let(:test_case) { build(:report_test_case, :failed) }
 
     subject { test_reports.with_attachment! }
 
@@ -126,8 +126,8 @@ RSpec.describe Gitlab::Ci::Reports::TestReports do
     end
 
     context 'when test suites contain an attachment' do
-      let(:test_case_succes) { build(:test_case) }
-      let(:test_case_with_attachment) { build(:test_case, :failed_with_attachment) }
+      let(:test_case_succes) { build(:report_test_case) }
+      let(:test_case_with_attachment) { build(:report_test_case, :failed_with_attachment) }
 
       before do
         test_reports.get_suite('rspec').add_test_case(test_case_succes)

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

@@ -91,7 +91,7 @@ RSpec.describe Gitlab::Ci::Reports::TestSuite do
     subject { test_suite.with_attachment! }
 
     context 'when test cases do not contain an attachment' do
-      let(:test_case) { build(:test_case, :failed)}
+      let(:test_case) { build(:report_test_case, :failed)}
 
       before do
         test_suite.add_test_case(test_case)
@@ -103,7 +103,7 @@ RSpec.describe Gitlab::Ci::Reports::TestSuite do
     end
 
     context 'when test cases contain an attachment' do
-      let(:test_case_with_attachment) { build(:test_case, :failed_with_attachment)}
+      let(:test_case_with_attachment) { build(:report_test_case, :failed_with_attachment)}
 
       before do
         test_suite.add_test_case(test_case_with_attachment)

spec/models/ci/test_case_failure_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::TestCaseFailure do
+  describe 'relationships' do
+    it { is_expected.to belong_to(:build) }
+    it { is_expected.to belong_to(:test_case) }
+  end
+
+  describe 'validations' do
+    subject { build(:ci_test_case_failure) }
+
+    it { is_expected.to validate_presence_of(:test_case) }
+    it { is_expected.to validate_presence_of(:build) }
+    it { is_expected.to validate_presence_of(:failed_at) }
+  end
+end

spec/models/ci/test_case_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::TestCase do
+  describe 'relationships' do
+    it { is_expected.to belong_to(:project) }
+    it { is_expected.to have_many(:test_case_failures) }
+  end
+
+  describe 'validations' do
+    subject { build(:ci_test_case) }
+
+    it { is_expected.to validate_presence_of(:project) }
+    it { is_expected.to validate_presence_of(:key_hash) }
+  end
+
+  describe '.find_or_create_by_batch' do
+    it 'finds or creates records for the given test case keys', :aggregate_failures do
+      project = create(:project)
+      existing_tc = create(:ci_test_case, project: project)
+      new_key = Digest::SHA256.hexdigest(SecureRandom.hex)
+      keys = [existing_tc.key_hash, new_key]
+
+      result = described_class.find_or_create_by_batch(project, keys)
+
+      expect(result.map(&:key_hash)).to match_array([existing_tc.key_hash, new_key])
+      expect(result).to all(be_persisted)
+    end
+  end
+end

spec/serializers/test_case_entity_spec.rb

@@ -42,7 +42,7 @@ RSpec.describe TestCaseEntity do
       end
 
       context 'when attachment is present' do
-        let(:test_case) { build(:test_case, :failed_with_attachment, job: job) }
+        let(:test_case) { build(:report_test_case, :failed_with_attachment, job: job) }
 
         it 'returns the attachment_url' do
           expect(subject).to include(:attachment_url)
@@ -50,7 +50,7 @@ RSpec.describe TestCaseEntity do
       end
 
       context 'when attachment is not present' do
-        let(:test_case) { build(:test_case, job: job) }
+        let(:test_case) { build(:report_test_case, job: job) }
 
         it 'returns a nil attachment_url' do
           expect(subject[:attachment_url]).to be_nil
@@ -64,7 +64,7 @@ RSpec.describe TestCaseEntity do
       end
 
       context 'when attachment is present' do
-        let(:test_case) { build(:test_case, :failed_with_attachment, job: job) }
+        let(:test_case) { build(:report_test_case, :failed_with_attachment, job: job) }
 
         it 'returns no attachment_url' do
           expect(subject).not_to include(:attachment_url)
@@ -72,7 +72,7 @@ RSpec.describe TestCaseEntity do
       end
 
       context 'when attachment is not present' do
-        let(:test_case) { build(:test_case, job: job) }
+        let(:test_case) { build(:report_test_case, job: job) }
 
         it 'returns no attachment_url' do
           expect(subject).not_to include(:attachment_url)

spec/services/ci/test_cases_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Ci::TestCasesService, :aggregate_failures do
+  describe '#execute' do
+    subject(:execute_service) { described_class.new.execute(build) }
+
+    context 'when build has test reports' do
+      let(:build) { create(:ci_build, :success, :test_reports) } # The test report has 2 test case failures
+
+      it 'creates test case failures records' do
+        execute_service
+
+        expect(Ci::TestCase.count).to eq(2)
+        expect(Ci::TestCaseFailure.count).to eq(2)
+      end
+
+      context 'when feature flag for test failure history is disabled' do
+        before do
+          stub_feature_flags(test_failure_history: false)
+        end
+
+        it 'does not persist data' do
+          execute_service
+
+          expect(Ci::TestCase.count).to eq(0)
+          expect(Ci::TestCaseFailure.count).to eq(0)
+        end
+      end
+
+      context 'when build is not for the default branch' do
+        before do
+          build.update_column(:ref, 'new-feature')
+        end
+
+        it 'does not persist data' do
+          execute_service
+
+          expect(Ci::TestCase.count).to eq(0)
+          expect(Ci::TestCaseFailure.count).to eq(0)
+        end
+      end
+
+      context 'when test failure data have already been persisted with the same exact attributes' do
+        before do
+          execute_service
+        end
+
+        it 'does not fail but does not persist new data' do
+          expect { described_class.new.execute(build) }.not_to raise_error
+
+          expect(Ci::TestCase.count).to eq(2)
+          expect(Ci::TestCaseFailure.count).to eq(2)
+        end
+      end
+
+      context 'when test failure data have duplicates within the same payload (happens when the JUnit report has duplicate test case names but have different failures)' do
+        let(:build) { create(:ci_build, :success, :test_reports_with_duplicate_failed_test_names) } # The test report has 2 test case failures but with the same test case keys
+
+        it 'does not fail but does not persist duplicate data' do
+          expect { described_class.new.execute(build) }.not_to raise_error
+
+          expect(Ci::TestCase.count).to eq(1)
+          expect(Ci::TestCaseFailure.count).to eq(1)
+        end
+      end
+
+      context 'when number of failed test cases exceed the limit' do
+        before do
+          stub_const("#{described_class.name}::MAX_TRACKABLE_FAILURES", 1)
+        end
+
+        it 'does not persist data' do
+          execute_service
+
+          expect(Ci::TestCase.count).to eq(0)
+          expect(Ci::TestCaseFailure.count).to eq(0)
+        end
+      end
+    end
+
+    context 'when build has no test reports' do
+      let(:build) { create(:ci_build, :running) }
+
+      it 'does not persist data' do
+        execute_service
+
+        expect(Ci::TestCase.count).to eq(0)
+        expect(Ci::TestCaseFailure.count).to eq(0)
+      end
+    end
+  end
+end

spec/workers/build_finished_worker_spec.rb

@@ -11,18 +11,28 @@ RSpec.describe BuildFinishedWorker do
     context 'when build exists' do
       let!(:build) { create(:ci_build) }
 
-      it 'calculates coverage and calls hooks' do
-        expect(BuildTraceSectionsWorker)
-          .to receive(:new).ordered.and_call_original
-        expect(BuildCoverageWorker)
-          .to receive(:new).ordered.and_call_original
-
-        expect_any_instance_of(BuildTraceSectionsWorker).to receive(:perform)
-        expect_any_instance_of(BuildCoverageWorker).to receive(:perform)
+      it 'calculates coverage and calls hooks', :aggregate_failures do
+        trace_worker = double('trace worker')
+        coverage_worker = double('coverage worker')
+
+        allow(BuildTraceSectionsWorker).to receive(:new).and_return(trace_worker)
+        allow(BuildCoverageWorker).to receive(:new).and_return(coverage_worker)
+
+        # Unfortunately, `ordered` does not seem to work when called within `allow_next_instance_of`
+        # so we're doing this the long and dirty way
+        expect(trace_worker).to receive(:perform).ordered
+        expect(coverage_worker).to receive(:perform).ordered
+
+        expect_next_instance_of(Ci::BuildReportResultWorker) do |instance|
+          expect(instance).to receive(:perform)
+        end
+        expect_next_instance_of(Ci::TestCasesService) do |instance|
+          expect(instance).to receive(:execute)
+        end
+
         expect(BuildHooksWorker).to receive(:perform_async)
         expect(ExpirePipelineCacheWorker).to receive(:perform_async)
         expect(ChatNotificationWorker).not_to receive(:perform_async)
-        expect(Ci::BuildReportResultWorker).not_to receive(:perform)
         expect(ArchiveTraceWorker).to receive(:perform_in)
 
         subject
@@ -31,7 +41,7 @@ RSpec.describe BuildFinishedWorker do
 
     context 'when build does not exist' do
       it 'does not raise exception' do
-        expect { described_class.new.perform(123) }
+        expect { described_class.new.perform(non_existing_record_id) }
           .not_to raise_error
       end
     end
@@ -45,17 +55,5 @@ RSpec.describe BuildFinishedWorker do
         subject
       end
     end
-
-    context 'when build has a test report' do
-      let(:build) { create(:ci_build, :test_reports) }
-
-      it 'schedules a BuildReportResult job' do
-        expect_next_instance_of(Ci::BuildReportResultWorker) do |worker|
-          expect(worker).to receive(:perform).with(build.id)
-        end
-
-        subject
-      end
-    end
   end
 end