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

6 files changed, 334 insertions, 12 deletions

diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb
index 375018e2229..d9bb3981539 100644
--- a/spec/controllers/projects/merge_requests_controller_spec.rb
+++ b/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
 
diff --git a/spec/features/merge_request/user_sees_merge_widget_spec.rb b/spec/features/merge_request/user_sees_merge_widget_spec.rb
index b6b3844f2ae..b285cd7a7ac 100644
--- a/spec/features/merge_request/user_sees_merge_widget_spec.rb
+++ b/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
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 3512ba6aee5..77b7332a761 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/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
diff --git a/spec/models/concerns/reactive_caching_spec.rb b/spec/models/concerns/reactive_caching_spec.rb
index 79f75c0ffa0..97a4c212f1c 100644
--- a/spec/models/concerns/reactive_caching_spec.rb
+++ b/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
 
diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb
index 3ab6a20cd55..6258bfa232f 100644
--- a/spec/models/merge_request_spec.rb
+++ b/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
 
diff --git a/spec/services/ci/compare_test_reports_service_spec.rb b/spec/services/ci/compare_test_reports_service_spec.rb
index d3bbf17cc5c..a26c970a8f0 100644
--- a/spec/services/ci/compare_test_reports_service_spec.rb
+++ b/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