Merge branch 'improve-junit-support-be' into 'master'

Improve JUnit test reports in merge request widgets

Closes #49966

See merge request gitlab-org/gitlab-ce!21039

app/controllers/projects/merge_requests_controller.rb

@@ -102,10 +102,10 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
   def test_reports
     result = @merge_request.compare_test_reports
 
-    Gitlab::PollingInterval.set_header(response, interval: 10_000)
-
     case result[:status]
     when :parsing
+      Gitlab::PollingInterval.set_header(response, interval: 3000)
+
       render json: '', status: :no_content
     when :parsed
       render json: result[:data].to_json, status: :ok

app/models/ci/pipeline.rb

@@ -606,12 +606,12 @@ module Ci
     end
 
     def has_test_reports?
-      complete? && builds.with_test_reports.any?
+      complete? && builds.latest.with_test_reports.any?
     end
 
     def test_reports
       Gitlab::Ci::Reports::TestReports.new.tap do |test_reports|
-        builds.with_test_reports.each do |build|
+        builds.latest.with_test_reports.each do |build|
           build.collect_test_reports!(test_reports)
         end
       end

app/models/concerns/reactive_caching.rb

@@ -42,6 +42,8 @@
 module ReactiveCaching
   extend ActiveSupport::Concern
 
+  InvalidateReactiveCache = Class.new(StandardError)
+
   included do
     class_attribute :reactive_cache_lease_timeout
 
@@ -63,15 +65,19 @@ module ReactiveCaching
     end
 
     def with_reactive_cache(*args, &blk)
-      bootstrap = !within_reactive_cache_lifetime?(*args)
-      Rails.cache.write(alive_reactive_cache_key(*args), true, expires_in: self.class.reactive_cache_lifetime)
+      unless within_reactive_cache_lifetime?(*args)
+        refresh_reactive_cache!(*args)
+        return nil
+      end
 
-      if bootstrap
-        ReactiveCachingWorker.perform_async(self.class, id, *args)
-        nil
-      else
+      keep_alive_reactive_cache!(*args)
+
+      begin
         data = Rails.cache.read(full_reactive_cache_key(*args))
         yield data if data.present?
+      rescue InvalidateReactiveCache
+        refresh_reactive_cache!(*args)
+        nil
       end
     end
 
@@ -96,6 +102,16 @@ module ReactiveCaching
 
     private
 
+    def refresh_reactive_cache!(*args)
+      clear_reactive_cache!(*args)
+      keep_alive_reactive_cache!(*args)
+      ReactiveCachingWorker.perform_async(self.class, id, *args)
+    end
+
+    def keep_alive_reactive_cache!(*args)
+      Rails.cache.write(alive_reactive_cache_key(*args), true, expires_in: self.class.reactive_cache_lifetime)
+    end
+
     def full_reactive_cache_key(*qualifiers)
       prefix = self.class.reactive_cache_key
       prefix = prefix.call(self) if prefix.respond_to?(:call)

app/models/merge_request.rb

@@ -16,8 +16,8 @@ class MergeRequest < ActiveRecord::Base
   include ReactiveCaching
 
   self.reactive_cache_key = ->(model) { [model.project.id, model.iid] }
-  self.reactive_cache_refresh_interval = 1.hour
-  self.reactive_cache_lifetime = 1.hour
+  self.reactive_cache_refresh_interval = 10.minutes
+  self.reactive_cache_lifetime = 10.minutes
 
   ignore_column :locked_at,
                 :ref_fetched,
@@ -1041,16 +1041,21 @@ class MergeRequest < ActiveRecord::Base
       return { status: :error, status_reason: 'This merge request does not have test reports' }
     end
 
-    with_reactive_cache(
-      :compare_test_results,
-      base_pipeline&.iid,
-      actual_head_pipeline.iid) { |data| data } || { status: :parsing }
+    with_reactive_cache(:compare_test_results) do |data|
+      unless Ci::CompareTestReportsService.new(project)
+        .latest?(base_pipeline, actual_head_pipeline, data)
+        raise InvalidateReactiveCache
+      end
+
+      data
+    end || { status: :parsing }
   end
 
   def calculate_reactive_cache(identifier, *args)
     case identifier.to_sym
     when :compare_test_results
-      Ci::CompareTestReportsService.new(project).execute(*args)
+      Ci::CompareTestReportsService.new(project).execute(
+        base_pipeline, actual_head_pipeline)
     else
       raise NotImplementedError, "Unknown identifier: #{identifier}"
     end

app/services/ci/compare_test_reports_service.rb

@@ -2,23 +2,36 @@
 
 module Ci
   class CompareTestReportsService < ::BaseService
-    def execute(base_pipeline_iid, head_pipeline_iid)
-      base_pipeline = project.pipelines.find_by_iid(base_pipeline_iid) if base_pipeline_iid
-      head_pipeline = project.pipelines.find_by_iid(head_pipeline_iid)
+    def execute(base_pipeline, head_pipeline)
+      comparer = Gitlab::Ci::Reports::TestReportsComparer
+        .new(base_pipeline&.test_reports, head_pipeline.test_reports)
 
-      begin
-        comparer = Gitlab::Ci::Reports::TestReportsComparer
-          .new(base_pipeline&.test_reports, head_pipeline.test_reports)
+      {
+        status: :parsed,
+        key: key(base_pipeline, head_pipeline),
+        data: TestReportsComparerSerializer
+          .new(project: project)
+          .represent(comparer).as_json
+      }
+    rescue => e
+      {
+        status: :error,
+        key: key(base_pipeline, head_pipeline),
+        status_reason: e.message
+      }
+    end
+
+    def latest?(base_pipeline, head_pipeline, data)
+      data&.fetch(:key, nil) == key(base_pipeline, head_pipeline)
+    end
+
+    private
 
-        {
-          status: :parsed,
-          data: TestReportsComparerSerializer
-            .new(project: project)
-            .represent(comparer).as_json
-        }
-      rescue => e
-        { status: :error, status_reason: e.message }
-      end
+    def key(base_pipeline, head_pipeline)
+      [
+        base_pipeline&.id, base_pipeline&.updated_at,
+        head_pipeline&.id, head_pipeline&.updated_at
+      ]
     end
   end
 end

changelogs/unreleased/improve-junit-support-be.yml

+---
+title: Improve JUnit test reports in merge request widgets
+merge_request: 49966
+author:
+type: fixed

spec/controllers/projects/merge_requests_controller_spec.rb

@@ -597,6 +597,12 @@ describe Projects::MergeRequestsController do
     context 'when comparison is being processed' do
       let(:comparison_status) { { status: :parsing } }
 
+      it 'sends polling interval' do
+        expect(Gitlab::PollingInterval).to receive(:set_header)
+
+        subject
+      end
+
       it 'returns 204 HTTP status' do
         subject
 
@@ -607,6 +613,12 @@ describe Projects::MergeRequestsController do
     context 'when comparison is done' do
       let(:comparison_status) { { status: :parsed, data: { summary: 1 } } }
 
+      it 'does not send polling interval' do
+        expect(Gitlab::PollingInterval).not_to receive(:set_header)
+
+        subject
+      end
+
       it 'returns 200 HTTP status' do
         subject
 
@@ -618,6 +630,12 @@ describe Projects::MergeRequestsController do
     context 'when user created corrupted test reports' do
       let(:comparison_status) { { status: :error, status_reason: 'Failed to parse test reports' } }
 
+      it 'does not send polling interval' do
+        expect(Gitlab::PollingInterval).not_to receive(:set_header)
+
+        subject
+      end
+
       it 'returns 400 HTTP status' do
         subject
 
@@ -629,6 +647,12 @@ describe Projects::MergeRequestsController do
     context 'when something went wrong on our system' do
       let(:comparison_status) { {} }
 
+      it 'does not send polling interval' do
+        expect(Gitlab::PollingInterval).not_to receive(:set_header)
+
+        subject
+      end
+
       it 'returns 500 HTTP status' do
         subject
 

spec/features/merge_request/user_sees_merge_widget_spec.rb

@@ -2,6 +2,7 @@ require 'rails_helper'
 
 describe 'Merge request > User sees merge widget', :js do
   include ProjectForksHelper
+  include TestReportsHelper
 
   let(:project) { create(:project, :repository) }
   let(:project_only_mwps) { create(:project, :repository, only_allow_merge_if_pipeline_succeeds: true) }
@@ -325,4 +326,229 @@ describe 'Merge request > User sees merge widget', :js do
       expect(page).to have_content('This merge request is in the process of being merged')
     end
   end
+
+  context 'when merge request has test reports' do
+    let!(:head_pipeline) do
+      create(:ci_pipeline,
+             :success,
+             project: project,
+             ref: merge_request.source_branch,
+             sha: merge_request.diff_head_sha)
+    end
+
+    let!(:build) { create(:ci_build, :success, pipeline: head_pipeline, project: project) }
+
+    before do
+      merge_request.update!(head_pipeline_id: head_pipeline.id)
+    end
+
+    context 'when result has not been parsed yet' do
+      let!(:job_artifact) { create(:ci_job_artifact, :junit, job: build, project: project) }
+
+      before do
+        visit project_merge_request_path(project, merge_request)
+      end
+
+      it 'shows parsing status' do
+        expect(page).to have_content('Test summary results are being parsed')
+      end
+    end
+
+    context 'when result has already been parsed' do
+      context 'when JUnit xml is correctly formatted' do
+        let!(:job_artifact) { create(:ci_job_artifact, :junit, job: build, project: project) }
+
+        before do
+          allow_any_instance_of(MergeRequest).to receive(:compare_test_reports).and_return(compared_data)
+
+          visit project_merge_request_path(project, merge_request)
+        end
+
+        it 'shows parsed results' do
+          expect(page).to have_content('Test summary contained')
+        end
+      end
+
+      context 'when JUnit xml is corrupted' do
+        let!(:job_artifact) { create(:ci_job_artifact, :junit_with_corrupted_data, job: build, project: project) }
+
+        before do
+          allow_any_instance_of(MergeRequest).to receive(:compare_test_reports).and_return(compared_data)
+
+          visit project_merge_request_path(project, merge_request)
+        end
+
+        it 'shows the error state' do
+          expect(page).to have_content('Test summary failed loading results')
+        end
+      end
+
+      def compared_data
+        Ci::CompareTestReportsService.new(project).execute(nil, head_pipeline)
+      end
+    end
+
+    context 'when test reports have been parsed correctly' do
+      let(:serialized_data) do
+        {
+          status: :parsed,
+          data: TestReportsComparerSerializer
+            .new(project: project)
+            .represent(comparer)
+        }
+      end
+
+      before do
+        allow_any_instance_of(MergeRequest)
+          .to receive(:has_test_reports?).and_return(true)
+        allow_any_instance_of(MergeRequest)
+          .to receive(:compare_test_reports).and_return(serialized_data)
+
+        visit project_merge_request_path(project, merge_request)
+      end
+
+      context 'when a new failures exists' do
+        let(:base_reports) do
+          Gitlab::Ci::Reports::TestReports.new.tap do |reports|
+            reports.get_suite('rspec').add_test_case(create_test_case_rspec_success)
+            reports.get_suite('junit').add_test_case(create_test_case_java_success)
+          end
+        end
+
+        let(:head_reports) do
+          Gitlab::Ci::Reports::TestReports.new.tap do |reports|
+            reports.get_suite('rspec').add_test_case(create_test_case_rspec_success)
+            reports.get_suite('junit').add_test_case(create_test_case_java_failed)
+          end
+        end
+
+        it 'shows test reports summary which includes the new failure' do
+          within(".mr-section-container") do
+            click_button 'Expand'
+
+            expect(page).to have_content('Test summary contained 1 failed test result out of 2 total tests')
+            within(".js-report-section-container") do
+              expect(page).to have_content('rspec found no changed test results out of 1 total test')
+              expect(page).to have_content('junit found 1 failed test result out of 1 total test')
+              expect(page).to have_content('New')
+              expect(page).to have_content('subtractTest')
+            end
+          end
+        end
+
+        context 'when user clicks the new failure' do
+          it 'shows the test report detail' do
+            within(".mr-section-container") do
+              click_button 'Expand'
+
+              within(".js-report-section-container") do
+                click_button 'subtractTest'
+
+                expect(page).to have_content('6.66')
+                expect(page).to have_content(sample_java_failed_message)
+              end
+            end
+          end
+        end
+      end
+
+      context 'when an existing failure exists' do
+        let(:base_reports) do
+          Gitlab::Ci::Reports::TestReports.new.tap do |reports|
+            reports.get_suite('rspec').add_test_case(create_test_case_rspec_failed)
+            reports.get_suite('junit').add_test_case(create_test_case_java_success)
+          end
+        end
+
+        let(:head_reports) do
+          Gitlab::Ci::Reports::TestReports.new.tap do |reports|
+            reports.get_suite('rspec').add_test_case(create_test_case_rspec_failed)
+            reports.get_suite('junit').add_test_case(create_test_case_java_success)
+          end
+        end
+
+        it 'shows test reports summary which includes the existing failure' do
+          within(".mr-section-container") do
+            click_button 'Expand'
+
+            expect(page).to have_content('Test summary contained 1 failed test result out of 2 total tests')
+            within(".js-report-section-container") do
+              expect(page).to have_content('rspec found 1 failed test result out of 1 total test')
+              expect(page).to have_content('junit found no changed test results out of 1 total test')
+              expect(page).not_to have_content('New')
+              expect(page).to have_content('Test#sum when a is 2 and b is 2 returns summary')
+            end
+          end
+        end
+
+        context 'when user clicks the existing failure' do
+          it 'shows test report detail of it' do
+            within(".mr-section-container") do
+              click_button 'Expand'
+
+              within(".js-report-section-container") do
+                click_button 'Test#sum when a is 2 and b is 2 returns summary'
+
+                expect(page).to have_content('2.22')
+                expect(page).to have_content(sample_rspec_failed_message)
+              end
+            end
+          end
+        end
+      end
+
+      context 'when a resolved failure exists' do
+        let(:base_reports) do
+          Gitlab::Ci::Reports::TestReports.new.tap do |reports|
+            reports.get_suite('rspec').add_test_case(create_test_case_rspec_success)
+            reports.get_suite('junit').add_test_case(create_test_case_java_failed)
+          end
+        end
+
+        let(:head_reports) do
+          Gitlab::Ci::Reports::TestReports.new.tap do |reports|
+            reports.get_suite('rspec').add_test_case(create_test_case_rspec_success)
+            reports.get_suite('junit').add_test_case(create_test_case_java_resolved)
+          end
+        end
+
+        let(:create_test_case_java_resolved) do
+          create_test_case_java_failed.tap do |test_case|
+            test_case.instance_variable_set("@status", Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS)
+          end
+        end
+
+        it 'shows test reports summary which includes the resolved failure' do
+          within(".mr-section-container") do
+            click_button 'Expand'
+
+            expect(page).to have_content('Test summary contained 1 fixed test result out of 2 total tests')
+            within(".js-report-section-container") do
+              expect(page).to have_content('rspec found no changed test results out of 1 total test')
+              expect(page).to have_content('junit found 1 fixed test result out of 1 total test')
+              expect(page).to have_content('subtractTest')
+            end
+          end
+        end
+
+        context 'when user clicks the resolved failure' do
+          it 'shows test report detail of it' do
+            within(".mr-section-container") do
+              click_button 'Expand'
+
+              within(".js-report-section-container") do
+                click_button 'subtractTest'
+
+                expect(page).to have_content('6.66')
+              end
+            end
+          end
+        end
+      end
+
+      def comparer
+        Gitlab::Ci::Reports::TestReportsComparer.new(base_reports, head_reports)
+      end
+    end
+  end
 end

spec/models/ci/pipeline_spec.rb

@@ -1856,9 +1856,7 @@ describe Ci::Pipeline, :mailer do
 
     context 'when pipeline has builds with test reports' do
       before do
-        create(:ci_build, pipeline: pipeline, project: project).tap do |build|
-          create(:ci_job_artifact, :junit, job: build, project: build.project)
-        end
+        create(:ci_build, :test_reports, pipeline: pipeline, project: project)
       end
 
       context 'when pipeline status is running' do
@@ -1875,6 +1873,22 @@ describe Ci::Pipeline, :mailer do
     end
 
     context 'when pipeline does not have builds with test reports' do
+      before do
+        create(:ci_build, :artifacts, pipeline: pipeline, project: project)
+      end
+
+      let(:pipeline) { create(:ci_pipeline, :success, project: project) }
+
+      it { is_expected.to be_falsey }
+    end
+
+    context 'when retried build has test reports' do
+      before do
+        create(:ci_build, :retried, :test_reports, pipeline: pipeline, project: project)
+      end
+
+      let(:pipeline) { create(:ci_pipeline, :success, project: project) }
+
       it { is_expected.to be_falsey }
     end
   end
@@ -1883,14 +1897,12 @@ describe Ci::Pipeline, :mailer do
     subject { pipeline.test_reports }
 
     context 'when pipeline has multiple builds with test reports' do
-      before do
-        create(:ci_build, :success, name: 'rspec', pipeline: pipeline, project: project).tap do |build|
-          create(:ci_job_artifact, :junit, job: build, project: build.project)
-        end
+      let!(:build_rspec) { create(:ci_build, :success, name: 'rspec', pipeline: pipeline, project: project) }
+      let!(:build_java) { create(:ci_build, :success, name: 'java', pipeline: pipeline, project: project) }
 
-        create(:ci_build, :success, name: 'java', pipeline: pipeline, project: project).tap do |build|
-          create(:ci_job_artifact, :junit_with_ant, job: build, project: build.project)
-        end
+      before do
+        create(:ci_job_artifact, :junit, job: build_rspec, project: project)
+        create(:ci_job_artifact, :junit_with_ant, job: build_java, project: project)
       end
 
       it 'returns test reports with collected data' do
@@ -1898,6 +1910,17 @@ describe Ci::Pipeline, :mailer do
         expect(subject.success_count).to be(5)
         expect(subject.failed_count).to be(2)
       end
+
+      context 'when builds are retried' do
+        let!(:build_rspec) { create(:ci_build, :retried, :success, name: 'rspec', pipeline: pipeline, project: project) }
+        let!(:build_java) { create(:ci_build, :retried, :success, name: 'java', pipeline: pipeline, project: project) }
+
+        it 'does not take retried builds into account' do
+          expect(subject.total_count).to be(0)
+          expect(subject.success_count).to be(0)
+          expect(subject.failed_count).to be(0)
+        end
+      end
     end
 
     context 'when pipeline does not have any builds with test reports' do

spec/models/concerns/reactive_caching_spec.rb

@@ -85,6 +85,14 @@ describe ReactiveCaching, :use_clean_rails_memory_store_caching do
 
         it { is_expected.to be_nil }
       end
+
+      context 'when cache was invalidated' do
+        it 'refreshes cache' do
+          expect(ReactiveCachingWorker).to receive(:perform_async).with(CacheTest, 666)
+
+          instance.with_reactive_cache { raise described_class::InvalidateReactiveCache }
+        end
+      end
     end
   end
 

spec/models/merge_request_spec.rb

@@ -1204,10 +1204,21 @@ describe MergeRequest do
 
         it 'returns status and data' do
           expect_any_instance_of(Ci::CompareTestReportsService)
-            .to receive(:execute).with(base_pipeline.iid, head_pipeline.iid)
+            .to receive(:execute).with(base_pipeline, head_pipeline).and_call_original
 
           subject
         end
+
+        context 'when cached results is not latest' do
+          before do
+            allow_any_instance_of(Ci::CompareTestReportsService)
+              .to receive(:latest?).and_return(false)
+          end
+
+          it 'raises and InvalidateReactiveCache error' do
+            expect { subject }.to raise_error(ReactiveCaching::InvalidateReactiveCache)
+          end
+        end
       end
     end
 

spec/services/ci/compare_test_reports_service_spec.rb

@@ -5,7 +5,7 @@ describe Ci::CompareTestReportsService do
   let(:project) { create(:project, :repository) }
 
   describe '#execute' do
-    subject { service.execute(base_pipeline&.iid, head_pipeline.iid) }
+    subject { service.execute(base_pipeline, head_pipeline) }
 
     context 'when head pipeline has test reports' do
       let!(:base_pipeline) { nil }
@@ -42,4 +42,34 @@ describe Ci::CompareTestReportsService do
       end
     end
   end
+
+  describe '#latest?' do
+    subject { service.latest?(base_pipeline, head_pipeline, data) }
+
+    let!(:base_pipeline) { nil }
+    let!(:head_pipeline) { create(:ci_pipeline, :with_test_reports, project: project) }
+    let!(:key) { service.send(:key, base_pipeline, head_pipeline) }
+
+    context 'when cache key is latest' do
+      let(:data) { { key: key } }
+
+      it { is_expected.to be_truthy }
+    end
+
+    context 'when cache key is outdated' do
+      before do
+        head_pipeline.update_column(:updated_at, 10.minutes.ago)
+      end
+
+      let(:data) { { key: key } }
+
+      it { is_expected.to be_falsy }
+    end
+
+    context 'when cache key is empty' do
+      let(:data) { { key: nil } }
+
+      it { is_expected.to be_falsy }
+    end
+  end
 end