Extend reports to support security features

app/models/ci/build.rb

@@ -68,8 +68,12 @@ module Ci
         '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').archive)
     end
 
+    scope :with_existing_job_artifacts, ->(query) do
+      where('EXISTS (?)', ::Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').merge(query))
+    end
+
     scope :with_archived_trace, ->() do
-      where('EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').trace)
+      with_existing_job_artifacts(Ci::JobArtifact.trace)
     end
 
     scope :without_archived_trace, ->() do
@@ -77,10 +81,12 @@ module Ci
     end
 
     scope :with_test_reports, ->() do
-      includes(:job_artifacts_junit) # Prevent N+1 problem when iterating each ci_job_artifact row
-        .where('EXISTS (?)', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id').test_reports)
+      with_existing_job_artifacts(Ci::JobArtifact.test_reports)
+        .eager_load_job_artifacts
     end
 
+    scope :eager_load_job_artifacts, -> { includes(:job_artifacts) }
+
     scope :with_artifacts_stored_locally, -> { with_artifacts_archive.where(artifacts_file_store: [nil, LegacyArtifactUploader::Store::LOCAL]) }
     scope :with_archived_trace_stored_locally, -> { with_archived_trace.where(artifacts_file_store: [nil, LegacyArtifactUploader::Store::LOCAL]) }
     scope :with_artifacts_not_expired, ->() { with_artifacts_archive.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) }
@@ -404,8 +410,8 @@ module Ci
       trace.exist?
     end
 
-    def has_test_reports?
-      job_artifacts.test_reports.any?
+    def has_job_artifacts?
+      job_artifacts.any?
     end
 
     def has_old_trace?
@@ -469,28 +475,23 @@ module Ci
       end
     end
 
-    def erase_artifacts!
-      remove_artifacts_file!
-      remove_artifacts_metadata!
-      save
-    end
-
-    def erase_test_reports!
-      # TODO: Use fast_destroy_all in the context of https://gitlab.com/gitlab-org/gitlab-ce/issues/35240
-      job_artifacts_junit&.destroy
+    # and use that for `ExpireBuildInstanceArtifactsWorker`?
+    def erase_erasable_artifacts!
+      job_artifacts.erasable.destroy_all # rubocop: disable DestroyAll
+      erase_old_artifacts!
     end
 
     def erase(opts = {})
       return false unless erasable?
 
-      erase_artifacts!
-      erase_test_reports!
+      job_artifacts.destroy_all # rubocop: disable DestroyAll
+      erase_old_artifacts!
       erase_trace!
       update_erased!(opts[:erased_by])
     end
 
     def erasable?
-      complete? && (artifacts? || has_test_reports? || has_trace?)
+      complete? && (artifacts? || has_job_artifacts? || has_trace?)
     end
 
     def erased?
@@ -648,8 +649,8 @@ module Ci
 
     def collect_test_reports!(test_reports)
       test_reports.get_suite(group_name).tap do |test_suite|
-        each_test_report do |file_type, blob|
-          Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, test_suite)
+        each_report(Ci::JobArtifact::TEST_REPORT_FILE_TYPES) do |file_type, blob|
+          Gitlab::Ci::Parsers::Test.fabricate!(file_type).parse!(blob, test_suite)
         end
       end
     end
@@ -669,6 +670,13 @@ module Ci
 
     private
 
+    def erase_old_artifacts!
+      # TODO: To be removed once we get rid of
+      remove_artifacts_file!
+      remove_artifacts_metadata!
+      save
+    end
+
     def successful_deployment_status
       if success? && last_deployment&.last?
         return :last
@@ -679,12 +687,17 @@ module Ci
       :creating
     end
 
-    def each_test_report
-      Ci::JobArtifact::TEST_REPORT_FILE_TYPES.each do |file_type|
-        public_send("job_artifacts_#{file_type}").each_blob do |blob| # rubocop:disable GitlabSecurity/PublicSend
-          yield file_type, blob
+    def each_report(report_types)
+      job_artifacts_for_types(report_types).each do |report_artifact|
+        report_artifact.each_blob do |blob|
+          yield report_artifact.file_type, blob
+        end
       end
     end
+
+    def job_artifacts_for_types(report_types)
+      # Use select to leverage cached associations and avoid N+1 queries
+      job_artifacts.select { |artifact| artifact.file_type.in?(report_types) }
     end
 
     def update_artifacts_size

app/models/ci/job_artifact.rb

@@ -9,8 +9,28 @@ module Ci
     NotSupportedAdapterError = Class.new(StandardError)
 
     TEST_REPORT_FILE_TYPES = %w[junit].freeze
-    DEFAULT_FILE_NAMES = { junit: 'junit.xml' }.freeze
-    TYPE_AND_FORMAT_PAIRS = { archive: :zip, metadata: :gzip, trace: :raw, junit: :gzip }.freeze
+    NON_ERASABLE_FILE_TYPES = %w[trace].freeze
+    DEFAULT_FILE_NAMES = {
+      archive: nil,
+      metadata: nil,
+      trace: nil,
+      junit: 'junit.xml',
+      sast: 'gl-sast-report.json',
+      dependency_scanning: 'gl-dependency-scanning-report.json',
+      container_scanning: 'gl-container-scanning-report.json',
+      dast: 'gl-dast-report.json'
+    }.freeze
+
+    TYPE_AND_FORMAT_PAIRS = {
+      archive: :zip,
+      metadata: :gzip,
+      trace: :raw,
+      junit: :gzip,
+      sast: :gzip,
+      dependency_scanning: :gzip,
+      container_scanning: :gzip,
+      dast: :gzip
+    }.freeze
 
     belongs_to :project
     belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id
@@ -27,8 +47,18 @@ module Ci
 
     scope :with_files_stored_locally, -> { where(file_store: [nil, ::JobArtifactUploader::Store::LOCAL]) }
 
+    scope :with_file_types, -> (file_types) do
+      types = self.file_types.select { |file_type| file_types.include?(file_type) }.values
+
+      where(file_type: types)
+    end
+
     scope :test_reports, -> do
-      types = self.file_types.select { |file_type| TEST_REPORT_FILE_TYPES.include?(file_type) }.values
+      with_file_types(TEST_REPORT_FILE_TYPES)
+    end
+
+    scope :erasable, -> do
+      types = self.file_types.reject { |file_type| NON_ERASABLE_FILE_TYPES.include?(file_type) }.values
 
       where(file_type: types)
     end
@@ -39,7 +69,11 @@ module Ci
       archive: 1,
       metadata: 2,
       trace: 3,
-      junit: 4
+      junit: 4,
+      sast: 5, ## EE-specific
+      dependency_scanning: 6, ## EE-specific
+      container_scanning: 7, ## EE-specific
+      dast: 8 ## EE-specific
     }
 
     enum file_format: {

app/workers/expire_build_instance_artifacts_worker.rb

@@ -13,7 +13,7 @@ class ExpireBuildInstanceArtifactsWorker
     return unless build&.project && !build.project.pending_delete
 
     Rails.logger.info "Removing artifacts for build #{build.id}..."
-    build.erase_artifacts!
+    build.erase_erasable_artifacts!
   end
   # rubocop: enable CodeReuse/ActiveRecord
 end

changelogs/unreleased/6717_extend_reports_for_security_products.yml

+---
+title: Extend reports feature to support Security Products
+merge_request: 21892
+author:
+type: added

lib/gitlab/ci/config/entry/reports.rb

+# frozen_string_literal: true
+
 module Gitlab
   module Ci
     class Config
@@ -9,7 +11,7 @@ module Gitlab
           include Validatable
           include Attributable
 
-          ALLOWED_KEYS = %i[junit].freeze
+          ALLOWED_KEYS = %i[junit sast dependency_scanning container_scanning dast].freeze
 
           attributes ALLOWED_KEYS
 
@@ -19,6 +21,10 @@ module Gitlab
 
             with_options allow_nil: true do
               validates :junit, array_of_strings_or_string: true
+              validates :sast, array_of_strings_or_string: true
+              validates :dependency_scanning, array_of_strings_or_string: true
+              validates :container_scanning, array_of_strings_or_string: true
+              validates :dast, array_of_strings_or_string: true
             end
           end
 

lib/gitlab/ci/parsers.rb

-module Gitlab
-  module Ci
-    module Parsers
-      def self.fabricate!(file_type)
-        "Gitlab::Ci::Parsers::#{file_type.classify}".constantize.new
-      end
-    end
-  end
-end

lib/gitlab/ci/parsers/junit.rb

-module Gitlab
-  module Ci
-    module Parsers
-      class Junit
-        JunitParserError = Class.new(StandardError)
-
-        def parse!(xml_data, test_suite)
-          root = Hash.from_xml(xml_data)
-
-          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}"
-        rescue => e
-          raise JunitParserError, "JUnit parsing failed: #{e.message}"
-        end
-
-        private
-
-        def all_cases(root, parent = nil, &blk)
-          return unless root.present?
-
-          [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 each_case(testcase, &blk)
-          return unless testcase.present?
-
-          [testcase].flatten.compact.map(&blk)
-        end
-
-        def create_test_case(data)
-          if data['failure']
-            status = ::Gitlab::Ci::Reports::TestCase::STATUS_FAILED
-            system_output = data['failure']
-          else
-            status = ::Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS
-            system_output = nil
-          end
-
-          ::Gitlab::Ci::Reports::TestCase.new(
-            classname: data['classname'],
-            name: data['name'],
-            file: data['file'],
-            execution_time: data['time'],
-            status: status,
-            system_output: system_output
-          )
-        end
-      end
-    end
-  end
-end

lib/gitlab/ci/parsers/test.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Parsers
+      module Test
+        ParserNotFoundError = Class.new(StandardError)
+
+        PARSERS = {
+          junit: ::Gitlab::Ci::Parsers::Test::Junit
+        }.freeze
+
+        def self.fabricate!(file_type)
+          PARSERS.fetch(file_type.to_sym).new
+        rescue KeyError
+          raise ParserNotFoundError, "Cannot find any parser matching file type '#{file_type}'"
+        end
+      end
+    end
+  end
+end

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

+# frozen_string_literal: true
+
+module Gitlab
+  module Ci
+    module Parsers
+      module Test
+        class Junit
+          JunitParserError = Class.new(StandardError)
+
+          def parse!(xml_data, test_suite)
+            root = Hash.from_xml(xml_data)
+
+            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}"
+          rescue => e
+            raise JunitParserError, "JUnit parsing failed: #{e.message}"
+          end
+
+          private
+
+          def all_cases(root, parent = nil, &blk)
+            return unless root.present?
+
+            [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 each_case(testcase, &blk)
+            return unless testcase.present?
+
+            [testcase].flatten.compact.map(&blk)
+          end
+
+          def create_test_case(data)
+            if data['failure']
+              status = ::Gitlab::Ci::Reports::TestCase::STATUS_FAILED
+              system_output = data['failure']
+            else
+              status = ::Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS
+              system_output = nil
+            end
+
+            ::Gitlab::Ci::Reports::TestCase.new(
+              classname: data['classname'],
+              name: data['name'],
+              file: data['file'],
+              execution_time: data['time'],
+              status: status,
+              system_output: system_output
+            )
+          end
+        end
+      end
+    end
+  end
+end

spec/factories/ci/job_artifacts.rb

@@ -14,6 +14,33 @@ FactoryBot.define do
       artifact.project ||= artifact.job.project
     end
 
+    trait :raw do
+      file_format :raw
+
+      after(:build) do |artifact, _|
+        artifact.file = fixture_file_upload(
+          Rails.root.join('spec/fixtures/trace/sample_trace'), 'text/plain')
+      end
+    end
+
+    trait :zip do
+      file_format :zip
+
+      after(:build) do |artifact, _|
+        artifact.file = fixture_file_upload(
+          Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip')
+      end
+    end
+
+    trait :gzip do
+      file_format :gzip
+
+      after(:build) do |artifact, _|
+        artifact.file = fixture_file_upload(
+          Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip')
+      end
+    end
+
     trait :archive do
       file_type :archive
       file_format :zip

spec/lib/gitlab/ci/config/entry/reports_spec.rb

@@ -3,13 +3,24 @@ require 'spec_helper'
 describe Gitlab::Ci::Config::Entry::Reports do
   let(:entry) { described_class.new(config) }
 
+  describe 'validates ALLOWED_KEYS' do
+    let(:artifact_file_types) { Ci::JobArtifact.file_types }
+
+    described_class::ALLOWED_KEYS.each do |keyword, _|
+      it "expects #{keyword} to be an artifact file_type" do
+        expect(artifact_file_types).to include(keyword)
+      end
+    end
+  end
+
   describe 'validation' do
     context 'when entry config value is correct' do
-      let(:config) { { junit: %w[junit.xml] } }
+      using RSpec::Parameterized::TableSyntax
 
+      shared_examples 'a valid entry' do |keyword, file|
         describe '#value' do
           it 'returns artifacs configuration' do
-          expect(entry.value).to eq config
+            expect(entry.value).to eq({ "#{keyword}": [file] } )
           end
         end
 
@@ -18,12 +29,27 @@ describe Gitlab::Ci::Config::Entry::Reports do
             expect(entry).to be_valid
           end
         end
+      end
+
+      where(:keyword, :file) do
+        :junit | 'junit.xml'
+        :sast | 'gl-sast-report.json'
+        :dependency_scanning | 'gl-dependency-scanning-report.json'
+        :container_scanning | 'gl-container-scanning-report.json'
+        :dast | 'gl-dast-report.json'
+      end
+
+      with_them do
+        context 'when value is an array' do
+          let(:config) { { "#{keyword}": [file] } }
+
+          it_behaves_like 'a valid entry', params[:keyword], params[:file]
+        end
 
         context 'when value is not array' do
-        let(:config) { { junit: 'junit.xml' } }
+          let(:config) { { "#{keyword}": file } }
 
-        it 'converts to array' do
-          expect(entry.value).to eq({ junit: ['junit.xml'] } )
+          it_behaves_like 'a valid entry', params[:keyword], params[:file]
         end
       end
     end
@@ -31,11 +57,13 @@ describe Gitlab::Ci::Config::Entry::Reports do
     context 'when entry value is not correct' do
       describe '#errors' do
         context 'when value of attribute is invalid' do
-          let(:config) { { junit: 10 } }
+          where(key: described_class::ALLOWED_KEYS) do
+            let(:config) { { "#{key}": 10 } }
 
             it 'reports error' do
               expect(entry.errors)
-              .to include 'reports junit should be an array of strings or a string'
+                .to include "reports #{key} should be an array of strings or a string"
+            end
           end
         end
 

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

 require 'fast_spec_helper'
 
-describe Gitlab::Ci::Parsers::Junit do
+describe Gitlab::Ci::Parsers::Test::Junit do
   describe '#parse!' do
     subject { described_class.new.parse!(junit, test_suite) }
 

spec/lib/gitlab/ci/parsers_spec.rb → spec/lib/gitlab/ci/parsers/test_spec.rb

 require 'spec_helper'
 
-describe Gitlab::Ci::Parsers do
+describe Gitlab::Ci::Parsers::Test do
   describe '.fabricate!' do
     subject { described_class.fabricate!(file_type) }
 
@@ -16,7 +16,7 @@ describe Gitlab::Ci::Parsers do
       let(:file_type) { 'undefined' }
 
       it 'raises an error' do
-        expect { subject }.to raise_error(NameError)
+        expect { subject }.to raise_error(Gitlab::Ci::Parsers::Test::ParserNotFoundError)
       end
     end
   end

spec/models/ci/build_spec.rb

@@ -176,9 +176,7 @@ describe Ci::Build do
       it 'does not execute a query for selecting job artifact one by one' do
         recorded = ActiveRecord::QueryRecorder.new do
           subject.each do |build|
-            Ci::JobArtifact::TEST_REPORT_FILE_TYPES.each do |file_type|
-              build.public_send("job_artifacts_#{file_type}").file.exists?
-            end
+            build.job_artifacts.map { |a| a.file.exists? }
           end
         end
 
@@ -550,44 +548,22 @@ describe Ci::Build do
     end
   end
 
-  describe '#has_test_reports?' do
-    subject { build.has_test_reports? }
+  describe '#has_job_artifacts?' do
+    subject { build.has_job_artifacts? }
 
-    context 'when build has a test report' do
-      let(:build) { create(:ci_build, :test_reports) }
+    context 'when build has a job artifact' do
+      let(:build) { create(:ci_build, :artifacts) }
 
       it { is_expected.to be_truthy }
     end
 
-    context 'when build does not have test reports' do
-      let(:build) { create(:ci_build, :artifacts) }
+    context 'when build does not have job artifacts' do
+      let(:build) { create(:ci_build, :legacy_artifacts) }
 
       it { is_expected.to be_falsy }
     end
   end
 
-  describe '#erase_test_reports!' do
-    subject { build.erase_test_reports! }
-
-    context 'when build has a test report' do
-      let!(:build) { create(:ci_build, :test_reports) }
-
-      it 'removes a test report' do
-        subject
-
-        expect(build.has_test_reports?).to be_falsy
-      end
-    end
-
-    context 'when build does not have test reports' do
-      let!(:build) { create(:ci_build, :artifacts) }
-
-      it 'does not erase anything' do
-        expect { subject }.not_to change { Ci::JobArtifact.count }
-      end
-    end
-  end
-
   describe '#has_old_trace?' do
     subject { build.has_old_trace? }
 
@@ -850,8 +826,8 @@ describe Ci::Build do
         expect(build.artifacts_metadata.exists?).to be_falsy
       end
 
-      it 'removes test reports' do
-        expect(build.job_artifacts.test_reports.count).to eq(0)
+      it 'removes all job_artifacts' do
+        expect(build.job_artifacts.count).to eq(0)
       end
 
       it 'erases build trace in trace file' do
@@ -1022,6 +998,32 @@ describe Ci::Build do
     end
   end
 
+  describe '#erase_erasable_artifacts!' do
+    let!(:build) { create(:ci_build, :success) }
+
+    subject { build.erase_erasable_artifacts! }
+
+    before do
+      Ci::JobArtifact.file_types.keys.each do |file_type|
+        create(:ci_job_artifact, job: build, file_type: file_type, file_format: Ci::JobArtifact::TYPE_AND_FORMAT_PAIRS[file_type.to_sym])
+      end
+    end
+
+    it "erases erasable artifacts" do
+      subject
+
+      expect(build.job_artifacts.erasable).to be_empty
+    end
+
+    it "keeps non erasable artifacts" do
+      subject
+
+      Ci::JobArtifact::NON_ERASABLE_FILE_TYPES.each do |file_type|
+        expect(build.send("job_artifacts_#{file_type}")).not_to be_nil
+      end
+    end
+  end
+
   describe '#first_pending' do
     let!(:first) { create(:ci_build, pipeline: pipeline, status: 'pending', created_at: Date.yesterday) }
     let!(:second) { create(:ci_build, pipeline: pipeline, status: 'pending') }
@@ -2844,16 +2846,10 @@ describe Ci::Build do
         end
 
         it 'raises an error' do
-          expect { subject }.to raise_error(Gitlab::Ci::Parsers::Junit::JunitParserError)
+          expect { subject }.to raise_error(Gitlab::Ci::Parsers::Test::Junit::JunitParserError)
         end
       end
     end
-
-    context 'when build does not have test reports' do
-      it 'raises an error' do
-        expect { subject }.to raise_error(NoMethodError)
-      end
-    end
   end
 
   describe '#artifacts_metadata_entry' do

spec/models/ci/job_artifact_spec.rb

@@ -31,6 +31,22 @@ describe Ci::JobArtifact do
     end
   end
 
+  describe '.erasable' do
+    subject { described_class.erasable }
+
+    context 'when there is am erasable artifact' do
+      let!(:artifact) { create(:ci_job_artifact, :junit) }
+
+      it { is_expected.to eq([artifact]) }
+    end
+
+    context 'when there are no erasable artifacts' do
+      let!(:artifact) { create(:ci_job_artifact, :trace) }
+
+      it { is_expected.to be_empty }
+    end
+  end
+
   describe 'callbacks' do
     subject { create(:ci_job_artifact, :archive) }
 
@@ -106,34 +122,46 @@ describe Ci::JobArtifact do
   describe 'validates file format' do
     subject { artifact }
 
-    context 'when archive type with zip format' do
-      let(:artifact) { build(:ci_job_artifact, :archive, file_format: :zip) }
+    described_class::TYPE_AND_FORMAT_PAIRS.except(:trace).each do |file_type, file_format|
+      context "when #{file_type} type with #{file_format} format" do
+        let(:artifact) { build(:ci_job_artifact, file_type: file_type, file_format: file_format) }
 
         it { is_expected.to be_valid }
       end
 
-    context 'when archive type with gzip format' do
-      let(:artifact) { build(:ci_job_artifact, :archive, file_format: :gzip) }
+      context "when #{file_type} type without format specification" do
+        let(:artifact) { build(:ci_job_artifact, file_type: file_type, file_format: nil) }
 
         it { is_expected.not_to be_valid }
       end
 
-    context 'when archive type without format specification' do
-      let(:artifact) { build(:ci_job_artifact, :archive, file_format: nil) }
+      context "when #{file_type} type with other formats" do
+        described_class.file_formats.except(file_format).values.each do |other_format|
+          let(:artifact) { build(:ci_job_artifact, file_type: file_type, file_format: other_format) }
 
           it { is_expected.not_to be_valid }
         end
+      end
+    end
+  end
 
-    context 'when junit type with zip format' do
-      let(:artifact) { build(:ci_job_artifact, :junit, file_format: :zip) }
+  describe 'validates DEFAULT_FILE_NAMES' do
+    subject { described_class::DEFAULT_FILE_NAMES }
 
-      it { is_expected.not_to be_valid }
+    described_class.file_types.each do |file_type, _|
+      it "expects #{file_type} to be included" do
+        is_expected.to include(file_type.to_sym)
+      end
+    end
   end
 
-    context 'when junit type with gzip format' do
-      let(:artifact) { build(:ci_job_artifact, :junit, file_format: :gzip) }
+  describe 'validates TYPE_AND_FORMAT_PAIRS' do
+    subject { described_class::TYPE_AND_FORMAT_PAIRS }
 
-      it { is_expected.to be_valid }
+    described_class.file_types.each do |file_type, _|
+      it "expects #{file_type} to be included" do
+        expect(described_class.file_formats).to include(subject[file_type.to_sym])
+      end
     end
   end
 

spec/presenters/ci/build_runner_presenter_spec.rb

@@ -3,7 +3,6 @@ require 'spec_helper'
 describe Ci::BuildRunnerPresenter do
   let(:presenter) { described_class.new(build) }
   let(:archive) { { paths: ['sample.txt'] } }
-  let(:junit) { { junit: ['junit.xml'] } }
 
   let(:archive_expectation) do
     {
@@ -14,16 +13,6 @@ describe Ci::BuildRunnerPresenter do
     }
   end
 
-  let(:junit_expectation) do
-    {
-      name: 'junit.xml',
-      artifact_type: :junit,
-      artifact_format: :gzip,
-      paths: ['junit.xml'],
-      when: 'always'
-    }
-  end
-
   describe '#artifacts' do
     context "when option contains archive-type artifacts" do
       let(:build) { create(:ci_build, options: { artifacts: archive } ) }
@@ -49,20 +38,44 @@ describe Ci::BuildRunnerPresenter do
       end
     end
 
-    context "when option has 'junit' keyword" do
-      let(:build) { create(:ci_build, options: { artifacts: { reports: junit } } ) }
+    context "with reports" do
+      Ci::JobArtifact::DEFAULT_FILE_NAMES.each do |file_type, filename|
+        let(:report) { { "#{file_type}": [filename] } }
+        let(:build) { create(:ci_build, options: { artifacts: { reports: report } } ) }
+
+        let(:report_expectation) do
+          {
+             name: filename,
+             artifact_type: :"#{file_type}",
+             artifact_format: :gzip,
+             paths: [filename],
+             when: 'always'
+          }
+        end
 
         it 'presents correct hash' do
-        expect(presenter.artifacts.first).to include(junit_expectation)
+          expect(presenter.artifacts.first).to include(report_expectation)
+        end
       end
     end
 
     context "when option has both archive and reports specification" do
-      let(:build) { create(:ci_build, options: { script: 'echo', artifacts: { **archive, reports: junit } } ) }
+      let(:report) { { junit: ['junit.xml'] } }
+      let(:build) { create(:ci_build, options: { script: 'echo', artifacts: { **archive, reports: report } } ) }
+
+      let(:report_expectation) do
+        {
+          name: 'junit.xml',
+          artifact_type: :junit,
+          artifact_format: :gzip,
+          paths: ['junit.xml'],
+          when: 'always'
+        }
+      end
 
       it 'presents correct hash' do
         expect(presenter.artifacts.first).to include(archive_expectation)
-        expect(presenter.artifacts.second).to include(junit_expectation)
+        expect(presenter.artifacts.second).to include(report_expectation)
       end
 
       context "when archive specifies 'expire_in' keyword" do
@@ -70,7 +83,7 @@ describe Ci::BuildRunnerPresenter do
 
         it 'inherits expire_in from archive' do
           expect(presenter.artifacts.first).to include({ **archive_expectation, expire_in: '3 mins 4 sec' })
-          expect(presenter.artifacts.second).to include({ **junit_expectation, expire_in: '3 mins 4 sec' })
+          expect(presenter.artifacts.second).to include({ **report_expectation, expire_in: '3 mins 4 sec' })
         end
       end
     end

spec/requests/api/jobs_spec.rb

@@ -721,7 +721,7 @@ describe API::Jobs do
         expect(job.trace.exist?).to be_falsy
         expect(job.artifacts_file.exists?).to be_falsy
         expect(job.artifacts_metadata.exists?).to be_falsy
-        expect(job.has_test_reports?).to be_falsy
+        expect(job.has_job_artifacts?).to be_falsy
       end
 
       it 'updates job' do

spec/services/ci/retry_build_service_spec.rb

@@ -24,7 +24,9 @@ describe Ci::RetryBuildService do
        artifacts_file artifacts_metadata artifacts_size created_at
        updated_at started_at finished_at queued_at erased_by
        erased_at auto_canceled_by job_artifacts job_artifacts_archive
-       job_artifacts_metadata job_artifacts_trace job_artifacts_junit].freeze
+       job_artifacts_metadata job_artifacts_trace job_artifacts_junit
+       job_artifacts_sast job_artifacts_dependency_scanning
+       job_artifacts_container_scanning job_artifacts_dast].freeze
 
   IGNORE_ACCESSORS =
     %i[type lock_version target_url base_tags trace_sections
@@ -38,9 +40,8 @@ describe Ci::RetryBuildService do
     let(:another_pipeline) { create(:ci_empty_pipeline, project: project) }
 
     let(:build) do
-      create(:ci_build, :failed, :artifacts, :test_reports, :expired, :erased,
-             :queued, :coverage, :tags, :allowed_to_fail, :on_tag,
-             :triggered, :trace_artifact, :teardown_environment,
+      create(:ci_build, :failed, :expired, :erased, :queued, :coverage, :tags,
+             :allowed_to_fail, :on_tag, :triggered, :teardown_environment,
              description: 'my-job', stage: 'test', stage_id: stage.id,
              pipeline: pipeline, auto_canceled_by: another_pipeline)
     end
@@ -50,6 +51,15 @@ describe Ci::RetryBuildService do
       # can reset one of the fields when assigning another. We plan to deprecate
       # and remove legacy `stage` column in the future.
       build.update(stage: 'test', stage_id: stage.id)
+
+      # Make sure we have one instance for every possible job_artifact_X
+      # associations to check they are correctly rejected on build duplication.
+      Ci::JobArtifact::TYPE_AND_FORMAT_PAIRS.each do |file_type, file_format|
+        create(:ci_job_artifact, file_format,
+               file_type: file_type, job: build, expire_at: build.artifacts_expire_at)
+      end
+
+      build.reload
     end
 
     describe 'clone accessors' do