Merge branch 'mo-expose-attachment-to-test-report' into 'master'

Expose attachment_url to TestCaseEntity

See merge request gitlab-org/gitlab!27243

app/models/ci/build.rb

@@ -873,7 +873,7 @@ module Ci
     def collect_test_reports!(test_reports)
       test_reports.get_suite(group_name).tap do |test_suite|
         each_report(Ci::JobArtifact::TEST_REPORT_FILE_TYPES) do |file_type, blob|
-          Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, test_suite)
+          Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, test_suite, job: self)
         end
       end
     end

app/models/ci/pipeline.rb

@@ -813,7 +813,7 @@ module Ci
 
     def test_reports
       Gitlab::Ci::Reports::TestReports.new.tap do |test_reports|
-        builds.latest.with_reports(Ci::JobArtifact.test_reports).each do |build|
+        builds.latest.with_reports(Ci::JobArtifact.test_reports).preload(:project).find_each do |build|
           build.collect_test_reports!(test_reports)
         end
       end

app/serializers/test_case_entity.rb

 # frozen_string_literal: true
 
 class TestCaseEntity < Grape::Entity
+  include API::Helpers::RelatedResourcesHelpers
+
   expose :status
   expose :name
   expose :classname
   expose :execution_time
   expose :system_output
   expose :stack_trace
+  expose :attachment_url, if: -> (*) { can_read_screenshots? } do |test_case|
+    expose_url(test_case.attachment_url)
+  end
+
+  private
+
+  alias_method :test_case, :object
+
+  def can_read_screenshots?
+    Feature.enabled?(:junit_pipeline_screenshots_view, options[:project]) && test_case.has_attachment?
+  end
 end

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

@@ -8,11 +8,11 @@ module Gitlab
           JunitParserError = Class.new(Gitlab::Ci::Parsers::ParserError)
           ATTACHMENT_TAG_REGEX = /\[\[ATTACHMENT\|(?<path>.+?)\]\]/.freeze
 
-          def parse!(xml_data, test_suite)
+          def parse!(xml_data, test_suite, **args)
             root = Hash.from_xml(xml_data)
 
             all_cases(root) do |test_case|
-              test_case = create_test_case(test_case)
+              test_case = create_test_case(test_case, args)
               test_suite.add_test_case(test_case)
             end
           rescue Nokogiri::XML::SyntaxError
@@ -46,7 +46,7 @@ module Gitlab
             [testcase].flatten.compact.map(&blk)
           end
 
-          def create_test_case(data)
+          def create_test_case(data, args)
             if data['failure']
               status = ::Gitlab::Ci::Reports::TestCase::STATUS_FAILED
               system_output = data['failure']
@@ -66,7 +66,8 @@ module Gitlab
               execution_time: data['time'],
               status: status,
               system_output: system_output,
-              attachment: attachment
+              attachment: attachment,
+              job: args.fetch(:job)
             )
           end
 

lib/gitlab/ci/reports/test_case.rb

@@ -10,9 +10,10 @@ module Gitlab
         STATUS_ERROR = 'error'
         STATUS_TYPES = [STATUS_SUCCESS, STATUS_FAILED, STATUS_SKIPPED, STATUS_ERROR].freeze
 
-        attr_reader :name, :classname, :execution_time, :status, :file, :system_output, :stack_trace, :key, :attachment
+        attr_reader :name, :classname, :execution_time, :status, :file, :system_output, :stack_trace, :key, :attachment, :job
 
-        def initialize(name:, classname:, execution_time:, status:, file: nil, system_output: nil, stack_trace: nil, attachment: nil)
+        # rubocop: disable Metrics/ParameterLists
+        def initialize(name:, classname:, execution_time:, status:, file: nil, system_output: nil, stack_trace: nil, attachment: nil, job: nil)
           @name = name
           @classname = classname
           @file = file
@@ -22,12 +23,24 @@ module Gitlab
           @stack_trace = stack_trace
           @key = sanitize_key_name("#{classname}_#{name}")
           @attachment = attachment
+          @job = job
         end
+        # rubocop: enable Metrics/ParameterLists
 
         def has_attachment?
           attachment.present?
         end
 
+        def attachment_url
+          return unless has_attachment?
+
+          Rails.application.routes.url_helpers.file_project_job_artifacts_path(
+            job.project,
+            job.id,
+            attachment
+          )
+        end
+
         private
 
         def sanitize_key_name(key)

spec/controllers/projects/pipelines_controller_spec.rb

@@ -705,13 +705,45 @@ describe Projects::PipelinesController do
   end
 
   describe 'GET test_report.json' do
-    subject(:get_test_report_json) do
-      get :test_report, params: {
-        namespace_id: project.namespace,
-        project_id: project,
-        id: pipeline.id
-      },
-      format: :json
+    let(:pipeline) { create(:ci_pipeline, project: project) }
+
+    context 'with attachments' do
+      let(:blob) do
+        <<~EOF
+          <testsuites>
+            <testsuite>
+              <testcase classname='Calculator' name='sumTest1' time='0.01'>
+                <failure>Some failure</failure>
+                <system-out>[[ATTACHMENT|some/path.png]]</system-out>
+              </testcase>
+            </testsuite>
+          </testsuites>
+        EOF
+      end
+
+      before do
+        allow_any_instance_of(Ci::JobArtifact).to receive(:each_blob).and_yield(blob)
+      end
+
+      it 'does not have N+1 problem with attachments' do
+        get_test_report_json
+
+        create(:ci_build, name: 'rspec', pipeline: pipeline).tap do |build|
+          create(:ci_job_artifact, :junit, job: build)
+        end
+
+        clear_controller_memoization
+
+        control_count = ActiveRecord::QueryRecorder.new { get_test_report_json }.count
+
+        create(:ci_build, name: 'karma', pipeline: pipeline).tap do |build|
+          create(:ci_job_artifact, :junit, job: build)
+        end
+
+        clear_controller_memoization
+
+        expect { get_test_report_json }.not_to exceed_query_limit(control_count)
+      end
     end
 
     context 'when feature is enabled' do
@@ -772,6 +804,20 @@ describe Projects::PipelinesController do
         expect(response.body).to be_empty
       end
     end
+
+    def get_test_report_json
+      get :test_report, params: {
+        namespace_id: project.namespace,
+        project_id: project,
+        id: pipeline.id
+      },
+      format: :json
+    end
+
+    def clear_controller_memoization
+      controller.clear_memoization(:pipeline_test_report)
+      controller.instance_variable_set(:@pipeline, nil)
+    end
   end
 
   describe 'GET test_report_count.json' do

spec/factories/ci/test_case.rb

@@ -9,6 +9,7 @@ FactoryBot.define do
     status { "success" }
     system_output { nil }
     attachment { nil }
+    association :job, factory: :ci_build
 
     trait :with_attachment do
       attachment { "some/path.png" }
@@ -24,7 +25,8 @@ FactoryBot.define do
         execution_time: execution_time,
         status: status,
         system_output: system_output,
-        attachment: attachment
+        attachment: attachment,
+        job: job
       )
     end
   end

spec/fixtures/api/schemas/entities/test_case.json

@@ -10,7 +10,8 @@
     "classname": { "type": "string" },
     "execution_time": { "type": "float" },
     "system_output": { "type": ["string", "null"] },
-    "stack_trace":  { "type": ["string", "null"] }
+    "stack_trace":  { "type": ["string", "null"] },
+    "attachment_url":  { "type": ["string", "null"] }
   },
   "additionalProperties": false
 }

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

@@ -4,10 +4,11 @@ require 'fast_spec_helper'
 
 describe Gitlab::Ci::Parsers::Test::Junit do
   describe '#parse!' do
-    subject { described_class.new.parse!(junit, test_suite) }
+    subject { described_class.new.parse!(junit, test_suite, args) }
 
     let(:test_suite) { Gitlab::Ci::Reports::TestSuite.new('rspec') }
     let(:test_cases) { flattened_test_cases(test_suite) }
+    let(:args) { { job: { id: 1, project: "project" } } }
 
     context 'when data is JUnit style XML' do
       context 'when there are no <testcases> in <testsuite>' do
@@ -205,7 +206,7 @@ describe Gitlab::Ci::Parsers::Test::Junit do
       end
     end
 
-    context 'when data contains an attachment tag' do
+    context 'when attachment is specified in failed test case' do
       let(:junit) do
         <<~EOF
           <testsuites>
@@ -219,11 +220,15 @@ describe Gitlab::Ci::Parsers::Test::Junit do
         EOF
       end
 
-      it 'add attachment to a test case' do
+      it 'assigns correct attributes to the test case' do
         expect { subject }.not_to raise_error
 
         expect(test_cases[0].has_attachment?).to be_truthy
         expect(test_cases[0].attachment).to eq("some/path.png")
+
+        expect(test_cases[0].job).to be_present
+        expect(test_cases[0].job[:id]).to eq(1)
+        expect(test_cases[0].job[:project]).to eq("project")
       end
     end
 

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

@@ -15,7 +15,8 @@ describe Gitlab::Ci::Reports::TestCase do
             file: 'spec/trace_spec.rb',
             execution_time: 1.23,
             status: described_class::STATUS_SUCCESS,
-            system_output: nil
+            system_output: nil,
+            job: build(:ci_build)
           }
         end
 
@@ -28,6 +29,7 @@ describe Gitlab::Ci::Reports::TestCase do
           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
 
@@ -99,6 +101,22 @@ describe Gitlab::Ci::Reports::TestCase do
       it '#has_attachment?' do
         expect(attachment_test_case.has_attachment?).to be_truthy
       end
+
+      it '#attachment_url' do
+        expect(attachment_test_case.attachment_url).to match(/file\/some\/path.png/)
+      end
+    end
+
+    context 'when attachment is missing' do
+      let(:test_case) { build(:test_case) }
+
+      it '#has_attachment?' do
+        expect(test_case.has_attachment?).to be_falsy
+      end
+
+      it '#attachment_url' do
+        expect(test_case.attachment_url).to be_nil
+      end
     end
   end
 end

spec/serializers/test_case_entity_spec.rb

@@ -31,5 +31,49 @@ describe TestCaseEntity do
         expect(subject[:execution_time]).to eq(2.22)
       end
     end
+
+    context 'when feature is enabled' do
+      before do
+        stub_feature_flags(junit_pipeline_screenshots_view: true)
+      end
+
+      context 'when attachment is present' do
+        let(:test_case) { build(:test_case, :with_attachment) }
+
+        it 'returns the attachment_url' do
+          expect(subject).to include(:attachment_url)
+        end
+      end
+
+      context 'when attachment is not present' do
+        let(:test_case) { build(:test_case) }
+
+        it 'returns a nil attachment_url' do
+          expect(subject[:attachment_url]).to be_nil
+        end
+      end
+    end
+
+    context 'when feature is disabled' do
+      before do
+        stub_feature_flags(junit_pipeline_screenshots_view: false)
+      end
+
+      context 'when attachment is present' do
+        let(:test_case) { build(:test_case, :with_attachment) }
+
+        it 'returns no attachment_url' do
+          expect(subject).not_to include(:attachment_url)
+        end
+      end
+
+      context 'when attachment is not present' do
+        let(:test_case) { build(:test_case) }
+
+        it 'returns no attachment_url' do
+          expect(subject).not_to include(:attachment_url)
+        end
+      end
+    end
   end
 end