From a08209ffa35a29cd84271895389b4537dee92e86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Trzci=C5=84ski?= Date: Tue, 2 Jul 2019 19:40:21 +0200 Subject: Limit amount of JUnit tests returned Currently, we do not cap amount of tests returned to frontend, thus in some extreme cases we can see a MBs of data stored in Redis. This adds an upper limit of 100 tests per-suite. We will continue showing the total counters correctly, but we will limit amount of tests that will be presented. --- .../merge_request/user_sees_merge_widget_spec.rb | 62 +++++++++++---- .../ci/reports/test_reports_comparer_spec.rb | 8 +- .../gitlab/ci/reports/test_suite_comparer_spec.rb | 23 ++---- spec/serializers/test_case_entity_spec.rb | 2 +- .../test_reports_comparer_entity_spec.rb | 8 +- .../test_reports_comparer_serializer_spec.rb | 8 +- .../serializers/test_suite_comparer_entity_spec.rb | 92 ++++++++++++++++++---- spec/support/test_reports/test_reports_helper.rb | 34 ++++---- 8 files changed, 153 insertions(+), 84 deletions(-) (limited to 'spec') 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 733e8aa3eba..30e30751693 100644 --- a/spec/features/merge_request/user_sees_merge_widget_spec.rb +++ b/spec/features/merge_request/user_sees_merge_widget_spec.rb @@ -519,6 +519,8 @@ describe 'Merge request > User sees merge widget', :js do end before do + allow_any_instance_of(TestSuiteComparerEntity) + .to receive(:max_tests).and_return(2) allow_any_instance_of(MergeRequest) .to receive(:has_test_reports?).and_return(true) allow_any_instance_of(MergeRequest) @@ -551,7 +553,7 @@ describe 'Merge request > User sees merge widget', :js 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') + expect(page).to have_content('addTest') end end end @@ -562,7 +564,7 @@ describe 'Merge request > User sees merge widget', :js do click_button 'Expand' within(".js-report-section-container") do - click_button 'subtractTest' + click_button 'addTest' expect(page).to have_content('6.66') expect(page).to have_content(sample_java_failed_message.gsub!(/\s+/, ' ').strip) @@ -596,7 +598,7 @@ describe 'Merge request > User sees merge widget', :js 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') + expect(page).to have_content('Test#sum when a is 1 and b is 3 returns summary') end end end @@ -607,7 +609,7 @@ describe 'Merge request > User sees merge widget', :js do click_button 'Expand' within(".js-report-section-container") do - click_button 'Test#sum when a is 2 and b is 2 returns summary' + click_button 'Test#sum when a is 1 and b is 3 returns summary' expect(page).to have_content('2.22') expect(page).to have_content(sample_rspec_failed_message.gsub!(/\s+/, ' ').strip) @@ -628,13 +630,7 @@ describe 'Merge request > User sees merge widget', :js do 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) + reports.get_suite('junit').add_test_case(create_test_case_java_success) end end @@ -646,7 +642,7 @@ describe 'Merge request > User sees merge widget', :js do 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') + expect(page).to have_content('addTest') end end end @@ -657,15 +653,53 @@ describe 'Merge request > User sees merge widget', :js do click_button 'Expand' within(".js-report-section-container") do - click_button 'subtractTest' + click_button 'addTest' - expect(page).to have_content('6.66') + expect(page).to have_content('5.55') end end end end end + context 'properly truncates the report' do + let(:base_reports) do + Gitlab::Ci::Reports::TestReports.new.tap do |reports| + 10.times do |index| + reports.get_suite('rspec').add_test_case( + create_test_case_rspec_failed(index)) + reports.get_suite('junit').add_test_case( + create_test_case_java_success(index)) + end + end + end + + let(:head_reports) do + Gitlab::Ci::Reports::TestReports.new.tap do |reports| + 10.times do |index| + reports.get_suite('rspec').add_test_case( + create_test_case_rspec_failed(index)) + reports.get_suite('junit').add_test_case( + create_test_case_java_failed(index)) + end + end + end + + it 'shows test reports summary which includes the resolved failure' do + within(".js-reports-container") do + click_button 'Expand' + + expect(page).to have_content('Test summary contained 20 failed test results out of 20 total tests') + within(".js-report-section-container") do + expect(page).to have_content('rspec found 10 failed test results out of 10 total tests') + expect(page).to have_content('junit found 10 failed test results out of 10 total tests') + + expect(page).to have_content('Test#sum when a is 1 and b is 3 returns summary', count: 2) + end + end + end + end + def comparer Gitlab::Ci::Reports::TestReportsComparer.new(base_reports, head_reports) end diff --git a/spec/lib/gitlab/ci/reports/test_reports_comparer_spec.rb b/spec/lib/gitlab/ci/reports/test_reports_comparer_spec.rb index 71c61e0345f..36582204cc1 100644 --- a/spec/lib/gitlab/ci/reports/test_reports_comparer_spec.rb +++ b/spec/lib/gitlab/ci/reports/test_reports_comparer_spec.rb @@ -74,17 +74,11 @@ describe Gitlab::Ci::Reports::TestReportsComparer do subject { comparer.resolved_count } context 'when there is a resolved test case in head suites' do - 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 - before do base_reports.get_suite('rspec').add_test_case(create_test_case_rspec_success) base_reports.get_suite('junit').add_test_case(create_test_case_java_failed) head_reports.get_suite('rspec').add_test_case(create_test_case_rspec_success) - head_reports.get_suite('junit').add_test_case(create_test_case_java_resolved) + head_reports.get_suite('junit').add_test_case(create_test_case_java_success) end it 'returns the correct count' do diff --git a/spec/lib/gitlab/ci/reports/test_suite_comparer_spec.rb b/spec/lib/gitlab/ci/reports/test_suite_comparer_spec.rb index 6ab16e5518d..579b3e6fd24 100644 --- a/spec/lib/gitlab/ci/reports/test_suite_comparer_spec.rb +++ b/spec/lib/gitlab/ci/reports/test_suite_comparer_spec.rb @@ -10,12 +10,6 @@ describe Gitlab::Ci::Reports::TestSuiteComparer do let(:test_case_success) { create_test_case_rspec_success } let(:test_case_failed) { create_test_case_rspec_failed } - let(:test_case_resolved) do - create_test_case_rspec_failed.tap do |test_case| - test_case.instance_variable_set("@status", Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS) - end - end - describe '#new_failures' do subject { comparer.new_failures } @@ -44,7 +38,7 @@ describe Gitlab::Ci::Reports::TestSuiteComparer do context 'when head sutie has a success test case which failed in base' do before do base_suite.add_test_case(test_case_failed) - head_suite.add_test_case(test_case_resolved) + head_suite.add_test_case(test_case_success) end it 'does not return the failed test case' do @@ -81,7 +75,7 @@ describe Gitlab::Ci::Reports::TestSuiteComparer do context 'when head sutie has a success test case which failed in base' do before do base_suite.add_test_case(test_case_failed) - head_suite.add_test_case(test_case_resolved) + head_suite.add_test_case(test_case_success) end it 'does not return the failed test case' do @@ -126,11 +120,11 @@ describe Gitlab::Ci::Reports::TestSuiteComparer do context 'when head sutie has a success test case which failed in base' do before do base_suite.add_test_case(test_case_failed) - head_suite.add_test_case(test_case_resolved) + head_suite.add_test_case(test_case_success) end it 'does not return the resolved test case' do - is_expected.to eq([test_case_resolved]) + is_expected.to eq([test_case_success]) end it 'returns the correct resolved count' do @@ -156,13 +150,8 @@ describe Gitlab::Ci::Reports::TestSuiteComparer do context 'when there are a new failure and an existing failure' do let(:test_case_1_success) { create_test_case_rspec_success } - let(:test_case_2_failed) { create_test_case_rspec_failed } - - let(:test_case_1_failed) do - create_test_case_rspec_success.tap do |test_case| - test_case.instance_variable_set("@status", Gitlab::Ci::Reports::TestCase::STATUS_FAILED) - end - end + let(:test_case_1_failed) { create_test_case_rspec_failed } + let(:test_case_2_failed) { create_test_case_rspec_failed('case2') } before do base_suite.add_test_case(test_case_1_success) diff --git a/spec/serializers/test_case_entity_spec.rb b/spec/serializers/test_case_entity_spec.rb index 986c9feb07b..cc5f086ca4e 100644 --- a/spec/serializers/test_case_entity_spec.rb +++ b/spec/serializers/test_case_entity_spec.rb @@ -24,7 +24,7 @@ describe TestCaseEntity do it 'contains correct test case details' do expect(subject[:status]).to eq('failed') - expect(subject[:name]).to eq('Test#sum when a is 2 and b is 2 returns summary') + expect(subject[:name]).to eq('Test#sum when a is 1 and b is 3 returns summary') expect(subject[:classname]).to eq('spec.test_spec') expect(subject[:execution_time]).to eq(2.22) end diff --git a/spec/serializers/test_reports_comparer_entity_spec.rb b/spec/serializers/test_reports_comparer_entity_spec.rb index 59c058fe368..4a951bbbde4 100644 --- a/spec/serializers/test_reports_comparer_entity_spec.rb +++ b/spec/serializers/test_reports_comparer_entity_spec.rb @@ -53,13 +53,7 @@ describe TestReportsComparerEntity do base_reports.get_suite('rspec').add_test_case(create_test_case_rspec_success) base_reports.get_suite('junit').add_test_case(create_test_case_java_failed) head_reports.get_suite('rspec').add_test_case(create_test_case_rspec_success) - head_reports.get_suite('junit').add_test_case(create_test_case_java_resolved) - 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 + head_reports.get_suite('junit').add_test_case(create_test_case_java_success) end it 'contains correct compared test reports details' do diff --git a/spec/serializers/test_reports_comparer_serializer_spec.rb b/spec/serializers/test_reports_comparer_serializer_spec.rb index 9ea86c0dd83..62dc6f486c5 100644 --- a/spec/serializers/test_reports_comparer_serializer_spec.rb +++ b/spec/serializers/test_reports_comparer_serializer_spec.rb @@ -44,13 +44,7 @@ describe TestReportsComparerSerializer do base_reports.get_suite('rspec').add_test_case(create_test_case_rspec_success) base_reports.get_suite('junit').add_test_case(create_test_case_java_failed) head_reports.get_suite('rspec').add_test_case(create_test_case_rspec_success) - head_reports.get_suite('junit').add_test_case(create_test_case_java_resolved) - 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 + head_reports.get_suite('junit').add_test_case(create_test_case_java_success) end it 'matches the schema' do diff --git a/spec/serializers/test_suite_comparer_entity_spec.rb b/spec/serializers/test_suite_comparer_entity_spec.rb index f61331f53a0..4b2cca2c68c 100644 --- a/spec/serializers/test_suite_comparer_entity_spec.rb +++ b/spec/serializers/test_suite_comparer_entity_spec.rb @@ -11,16 +11,10 @@ describe TestSuiteComparerEntity do let(:test_case_success) { create_test_case_rspec_success } let(:test_case_failed) { create_test_case_rspec_failed } - let(:test_case_resolved) do - create_test_case_rspec_failed.tap do |test_case| - test_case.instance_variable_set("@status", Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS) - end - end - describe '#as_json' do subject { entity.as_json } - context 'when head sutie has a newly failed test case which does not exist in base' do + context 'when head suite has a newly failed test case which does not exist in base' do before do base_suite.add_test_case(test_case_success) head_suite.add_test_case(test_case_failed) @@ -41,7 +35,7 @@ describe TestSuiteComparerEntity do end end - context 'when head sutie still has a failed test case which failed in base' do + context 'when head suite still has a failed test case which failed in base' do before do base_suite.add_test_case(test_case_failed) head_suite.add_test_case(test_case_failed) @@ -62,10 +56,10 @@ describe TestSuiteComparerEntity do end end - context 'when head sutie has a success test case which failed in base' do + context 'when head suite has a success test case which failed in base' do before do base_suite.add_test_case(test_case_failed) - head_suite.add_test_case(test_case_resolved) + head_suite.add_test_case(test_case_success) end it 'contains correct compared test suite details' do @@ -74,13 +68,83 @@ describe TestSuiteComparerEntity do expect(subject[:summary]).to include(total: 1, resolved: 1, failed: 0) expect(subject[:new_failures]).to be_empty subject[:resolved_failures].first.tap do |resolved_failure| - expect(resolved_failure[:status]).to eq(test_case_resolved.status) - expect(resolved_failure[:name]).to eq(test_case_resolved.name) - expect(resolved_failure[:execution_time]).to eq(test_case_resolved.execution_time) - expect(resolved_failure[:system_output]).to eq(test_case_resolved.system_output) + expect(resolved_failure[:status]).to eq(test_case_success.status) + expect(resolved_failure[:name]).to eq(test_case_success.name) + expect(resolved_failure[:execution_time]).to eq(test_case_success.execution_time) + expect(resolved_failure[:system_output]).to eq(test_case_success.system_output) end expect(subject[:existing_failures]).to be_empty end end + + context 'limits amount of tests returned' do + before do + stub_const('TestSuiteComparerEntity::DEFAULT_MAX_TESTS', 2) + stub_const('TestSuiteComparerEntity::DEFAULT_MIN_TESTS', 1) + end + + context 'prefers new over existing and resolved' do + before do + 3.times { add_new_failure } + 3.times { add_existing_failure } + 3.times { add_resolved_failure } + end + + it 'returns 2 new failures, and 1 of resolved and existing' do + expect(subject[:summary]).to include(total: 9, resolved: 3, failed: 6) + expect(subject[:new_failures].count).to eq(2) + expect(subject[:existing_failures].count).to eq(1) + expect(subject[:resolved_failures].count).to eq(1) + end + end + + context 'prefers existing over resolved' do + before do + 3.times { add_existing_failure } + 3.times { add_resolved_failure } + end + + it 'returns 2 existing failures, and 1 resolved' do + expect(subject[:summary]).to include(total: 6, resolved: 3, failed: 3) + expect(subject[:new_failures].count).to eq(0) + expect(subject[:existing_failures].count).to eq(2) + expect(subject[:resolved_failures].count).to eq(1) + end + end + + context 'limits amount of resolved' do + before do + 3.times { add_resolved_failure } + end + + it 'returns 2 resolved failures' do + expect(subject[:summary]).to include(total: 3, resolved: 3, failed: 0) + expect(subject[:new_failures].count).to eq(0) + expect(subject[:existing_failures].count).to eq(0) + expect(subject[:resolved_failures].count).to eq(2) + end + end + + private + + def add_new_failure + failed_case = create_test_case_rspec_failed(SecureRandom.hex) + head_suite.add_test_case(failed_case) + end + + def add_existing_failure + failed_case = create_test_case_rspec_failed(SecureRandom.hex) + base_suite.add_test_case(failed_case) + head_suite.add_test_case(failed_case) + end + + def add_resolved_failure + case_name = SecureRandom.hex + failed_case = create_test_case_rspec_failed(case_name) + success_case = create_test_case_rspec_success(case_name) + base_suite.add_test_case(failed_case) + head_suite.add_test_case(success_case) + end + end end end diff --git a/spec/support/test_reports/test_reports_helper.rb b/spec/support/test_reports/test_reports_helper.rb index 45c6e04dbf3..6840fb9a860 100644 --- a/spec/support/test_reports/test_reports_helper.rb +++ b/spec/support/test_reports/test_reports_helper.rb @@ -1,36 +1,36 @@ module TestReportsHelper - def create_test_case_rspec_success + def create_test_case_rspec_success(name = 'test_spec') Gitlab::Ci::Reports::TestCase.new( name: 'Test#sum when a is 1 and b is 3 returns summary', - classname: 'spec.test_spec', + classname: "spec.#{name}", file: './spec/test_spec.rb', execution_time: 1.11, status: Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS) end - def create_test_case_rspec_failed + def create_test_case_rspec_failed(name = 'test_spec') Gitlab::Ci::Reports::TestCase.new( - name: 'Test#sum when a is 2 and b is 2 returns summary', - classname: 'spec.test_spec', + name: 'Test#sum when a is 1 and b is 3 returns summary', + classname: "spec.#{name}", file: './spec/test_spec.rb', execution_time: 2.22, system_output: sample_rspec_failed_message, status: Gitlab::Ci::Reports::TestCase::STATUS_FAILED) end - def create_test_case_rspec_skipped + def create_test_case_rspec_skipped(name = 'test_spec') Gitlab::Ci::Reports::TestCase.new( name: 'Test#sum when a is 3 and b is 3 returns summary', - classname: 'spec.test_spec', + classname: "spec.#{name}", file: './spec/test_spec.rb', execution_time: 3.33, status: Gitlab::Ci::Reports::TestCase::STATUS_SKIPPED) end - def create_test_case_rspec_error + def create_test_case_rspec_error(name = 'test_spec') Gitlab::Ci::Reports::TestCase.new( name: 'Test#sum when a is 4 and b is 4 returns summary', - classname: 'spec.test_spec', + classname: "spec.#{name}", file: './spec/test_spec.rb', execution_time: 4.44, status: Gitlab::Ci::Reports::TestCase::STATUS_ERROR) @@ -48,34 +48,34 @@ module TestReportsHelper EOF end - def create_test_case_java_success + def create_test_case_java_success(name = 'addTest') Gitlab::Ci::Reports::TestCase.new( - name: 'addTest', + name: name, classname: 'CalculatorTest', execution_time: 5.55, status: Gitlab::Ci::Reports::TestCase::STATUS_SUCCESS) end - def create_test_case_java_failed + def create_test_case_java_failed(name = 'addTest') Gitlab::Ci::Reports::TestCase.new( - name: 'subtractTest', + name: name, classname: 'CalculatorTest', execution_time: 6.66, system_output: sample_java_failed_message, status: Gitlab::Ci::Reports::TestCase::STATUS_FAILED) end - def create_test_case_java_skipped + def create_test_case_java_skipped(name = 'addTest') Gitlab::Ci::Reports::TestCase.new( - name: 'multiplyTest', + name: name, classname: 'CalculatorTest', execution_time: 7.77, status: Gitlab::Ci::Reports::TestCase::STATUS_SKIPPED) end - def create_test_case_java_error + def create_test_case_java_error(name = 'addTest') Gitlab::Ci::Reports::TestCase.new( - name: 'divideTest', + name: name, classname: 'CalculatorTest', execution_time: 8.88, status: Gitlab::Ci::Reports::TestCase::STATUS_ERROR) -- cgit v1.2.3