summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/controllers/projects/merge_requests_controller.rb4
-rw-r--r--app/models/ci/pipeline.rb4
-rw-r--r--app/models/concerns/reactive_caching.rb28
-rw-r--r--app/models/merge_request.rb19
-rw-r--r--app/services/ci/compare_test_reports_service.rb43
-rw-r--r--changelogs/unreleased/improve-junit-support-be.yml5
-rw-r--r--spec/controllers/projects/merge_requests_controller_spec.rb24
-rw-r--r--spec/features/merge_request/user_sees_merge_widget_spec.rb226
-rw-r--r--spec/models/ci/pipeline_spec.rb43
-rw-r--r--spec/models/concerns/reactive_caching_spec.rb8
-rw-r--r--spec/models/merge_request_spec.rb13
-rw-r--r--spec/services/ci/compare_test_reports_service_spec.rb32
12 files changed, 405 insertions, 44 deletions
diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb
index eaf4434f913..1b069fe507b 100644
--- a/app/controllers/projects/merge_requests_controller.rb
+++ b/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
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 75dfa00d12e..e4aed76f611 100644
--- a/app/models/ci/pipeline.rb
+++ b/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
diff --git a/app/models/concerns/reactive_caching.rb b/app/models/concerns/reactive_caching.rb
index 9155d82d567..65cc7a751f9 100644
--- a/app/models/concerns/reactive_caching.rb
+++ b/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)
diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb
index 9b3e2d4446d..396647a14ae 100644
--- a/app/models/merge_request.rb
+++ b/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
diff --git a/app/services/ci/compare_test_reports_service.rb b/app/services/ci/compare_test_reports_service.rb
index 7a112211d94..ec25e934a27 100644
--- a/app/services/ci/compare_test_reports_service.rb
+++ b/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
diff --git a/changelogs/unreleased/improve-junit-support-be.yml b/changelogs/unreleased/improve-junit-support-be.yml
new file mode 100644
index 00000000000..db4d47caa7c
--- /dev/null
+++ b/changelogs/unreleased/improve-junit-support-be.yml
@@ -0,0 +1,5 @@
+---
+title: Improve JUnit test reports in merge request widgets
+merge_request: 49966
+author:
+type: fixed
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
View Lorry Controller Status