From e05c0831611942a4723d3810deb066def521aa27 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Thu, 2 May 2019 18:10:35 +0200 Subject: [PATCH 1/3] Fix log info when measured time of tests was lost --- lib/knapsack_pro/report.rb | 20 +++++----- .../runners/queue/minitest_runner.rb | 2 +- .../runners/queue/rspec_runner.rb | 2 +- spec/knapsack_pro/report_spec.rb | 40 ++++++++++++------- .../runners/queue/minitest_runner_spec.rb | 2 +- .../runners/queue/rspec_runner_spec.rb | 4 +- 6 files changed, 41 insertions(+), 29 deletions(-) diff --git a/lib/knapsack_pro/report.rb b/lib/knapsack_pro/report.rb index fb6c3e06..6d46ba2b 100644 --- a/lib/knapsack_pro/report.rb +++ b/lib/knapsack_pro/report.rb @@ -27,7 +27,7 @@ def self.save_subset_queue_to_file end end - def self.save_node_queue_to_api(executed_test_files_count) + def self.save_node_queue_to_api test_files = [] Dir.glob("#{queue_path}/*.json").each do |file| report = JSON.parse(File.read(file)) @@ -35,15 +35,17 @@ def self.save_node_queue_to_api(executed_test_files_count) end if test_files.empty? - if executed_test_files_count == 0 - KnapsackPro.logger.warn("No test files were executed on this CI node.") - KnapsackPro.logger.debug("When you use knapsack_pro queue mode then probably reason might be that CI node was started after the test files from the queue were already executed by other CI nodes. That is why this CI node has no test files to execute.") - KnapsackPro.logger.debug("Another reason might be when your CI node failed in a way that prevented knapsack_pro to save time execution data to Knapsack Pro API and you have just tried to retry failed CI node but instead you got no test files to execute. In that case knapsack_pro don't know what testes should be executed here.") - end + KnapsackPro.logger.warn("No test files were executed on this CI node.") + KnapsackPro.logger.debug("When you use knapsack_pro queue mode then probably reason might be that CI node was started after the test files from the queue were already executed by other CI nodes. That is why this CI node has no test files to execute.") + KnapsackPro.logger.debug("Another reason might be when your CI node failed in a way that prevented knapsack_pro to save time execution data to Knapsack Pro API and you have just tried to retry failed CI node but instead you got no test files to execute. In that case knapsack_pro don't know what testes should be executed here.") + end + + measured_test_files = test_files + .map { |t| t['time_execution'] } + .select { |time_execution| time_execution != KnapsackPro::Tracker::DEFAULT_TEST_FILE_TIME } - if executed_test_files_count > 0 - KnapsackPro.logger.error("#{executed_test_files_count} test files were executed on this CI node but the recorded time of it was lost. Probably you have a code (i.e. RSpec hooks) that clears tmp directory in your project. Please ensure you do not remove the content of tmp/knapsack_pro/queue/ directory between tests run. Another reason might be that you forgot to add Knapsack::Adapters::RspecAdapter.bind in your rails_helper.rb or spec_helper.rb. Please follow the installation guide again: https://docs.knapsackpro.com/integration/") - end + if test_files.size > 0 && measured_test_files.size == 0 + KnapsackPro.logger.error("#{test_files.size} test files were executed on this CI node but the recorded time of it was lost. Probably you have a code (i.e. RSpec hooks) that clears tmp directory in your project. Please ensure you do not remove the content of tmp/knapsack_pro/queue/ directory between tests run. Another reason might be that you forgot to add Knapsack::Adapters::RspecAdapter.bind in your rails_helper.rb or spec_helper.rb. Please follow the installation guide again: https://docs.knapsackpro.com/integration/") end create_build_subset(test_files) diff --git a/lib/knapsack_pro/runners/queue/minitest_runner.rb b/lib/knapsack_pro/runners/queue/minitest_runner.rb index e6e20096..2f5bbc47 100644 --- a/lib/knapsack_pro/runners/queue/minitest_runner.rb +++ b/lib/knapsack_pro/runners/queue/minitest_runner.rb @@ -48,7 +48,7 @@ def self.run_tests(accumulator) if test_file_paths.empty? KnapsackPro::Hooks::Queue.call_after_queue - KnapsackPro::Report.save_node_queue_to_api(all_test_file_paths.count) + KnapsackPro::Report.save_node_queue_to_api return { status: :completed, diff --git a/lib/knapsack_pro/runners/queue/rspec_runner.rb b/lib/knapsack_pro/runners/queue/rspec_runner.rb index 616a7660..68d03966 100644 --- a/lib/knapsack_pro/runners/queue/rspec_runner.rb +++ b/lib/knapsack_pro/runners/queue/rspec_runner.rb @@ -59,7 +59,7 @@ def self.run_tests(accumulator) KnapsackPro::Hooks::Queue.call_after_queue - KnapsackPro::Report.save_node_queue_to_api(all_test_file_paths.count) + KnapsackPro::Report.save_node_queue_to_api return { status: :completed, diff --git a/spec/knapsack_pro/report_spec.rb b/spec/knapsack_pro/report_spec.rb index 270e6c54..c422f69c 100644 --- a/spec/knapsack_pro/report_spec.rb +++ b/spec/knapsack_pro/report_spec.rb @@ -43,16 +43,14 @@ end describe '.save_node_queue_to_api' do - context 'when json files with recorded time exist for executed test files' do + context 'when json files with recorded time does exist and test files have measured and default time execution' do let(:json_test_file_a_path) { double } - let(:json_test_file_a) { [{ 'path' => 'a_spec.rb' }] } + let(:json_test_file_a) { [{ 'path' => 'a_spec.rb', 'time_execution' => 0.1234 }] } let(:json_test_file_b_path) { double } - let(:json_test_file_b) { [{ 'path' => 'b_spec.rb' }] } + let(:json_test_file_b) { [{ 'path' => 'b_spec.rb', 'time_execution' => KnapsackPro::Tracker::DEFAULT_TEST_FILE_TIME }] } - let(:executed_test_files_count) { 2 } - - subject { described_class.save_node_queue_to_api(executed_test_files_count) } + subject { described_class.save_node_queue_to_api } before do queue_id = 'fake-queue-id' @@ -68,6 +66,8 @@ end it 'creates build subset for 2 recorded test files timing' do + expect(KnapsackPro).not_to receive(:logger) + expect(described_class).to receive(:create_build_subset).with( json_test_file_a + json_test_file_b ) @@ -76,16 +76,26 @@ end end - context 'when json files with recorded time does not exist for executed test files' do - let(:executed_test_files_count) { 2 } + context 'when json files with recorded time does exist and all test files have default time execution' do + let(:json_test_file_a_path) { double } + let(:json_test_file_a) { [{ 'path' => 'a_spec.rb', 'time_execution' => KnapsackPro::Tracker::DEFAULT_TEST_FILE_TIME }] } + + let(:json_test_file_b_path) { double } + let(:json_test_file_b) { [{ 'path' => 'b_spec.rb', 'time_execution' => KnapsackPro::Tracker::DEFAULT_TEST_FILE_TIME }] } - subject { described_class.save_node_queue_to_api(executed_test_files_count) } + subject { described_class.save_node_queue_to_api } before do queue_id = 'fake-queue-id' expect(KnapsackPro::Config::Env).to receive(:queue_id).and_return(queue_id) - expect(Dir).to receive(:glob).with('tmp/knapsack_pro/queue/fake-queue-id/*.json').and_return([]) + expect(Dir).to receive(:glob).with('tmp/knapsack_pro/queue/fake-queue-id/*.json').and_return([ + json_test_file_a_path, + json_test_file_b_path + ]) + + expect(File).to receive(:read).with(json_test_file_a_path).and_return(json_test_file_a.to_json) + expect(File).to receive(:read).with(json_test_file_b_path).and_return(json_test_file_b.to_json) end it 'logs error on lost info about recorded timing for test files due missing json files AND creates empty build subset' do @@ -93,16 +103,16 @@ expect(KnapsackPro).to receive(:logger).and_return(logger) expect(logger).to receive(:error).with('2 test files were executed on this CI node but the recorded time of it was lost. Probably you have a code (i.e. RSpec hooks) that clears tmp directory in your project. Please ensure you do not remove the content of tmp/knapsack_pro/queue/ directory between tests run. Another reason might be that you forgot to add Knapsack::Adapters::RspecAdapter.bind in your rails_helper.rb or spec_helper.rb. Please follow the installation guide again: https://docs.knapsackpro.com/integration/') - expect(described_class).to receive(:create_build_subset).with([]) + expect(described_class).to receive(:create_build_subset).with( + json_test_file_a + json_test_file_b + ) subject end end - context 'when json files with recorded time does not exist AND no executed test files' do - let(:executed_test_files_count) { 0 } - - subject { described_class.save_node_queue_to_api(executed_test_files_count) } + context 'when json files with recorded time does not exist' do + subject { described_class.save_node_queue_to_api } before do queue_id = 'fake-queue-id' diff --git a/spec/knapsack_pro/runners/queue/minitest_runner_spec.rb b/spec/knapsack_pro/runners/queue/minitest_runner_spec.rb index c46e1a92..43c8e888 100644 --- a/spec/knapsack_pro/runners/queue/minitest_runner_spec.rb +++ b/spec/knapsack_pro/runners/queue/minitest_runner_spec.rb @@ -161,7 +161,7 @@ it 'returns exit code 0' do expect(KnapsackPro::Hooks::Queue).to receive(:call_after_queue) - expect(KnapsackPro::Report).to receive(:save_node_queue_to_api).with(0) + expect(KnapsackPro::Report).to receive(:save_node_queue_to_api) expect(subject).to eq({ status: :completed, diff --git a/spec/knapsack_pro/runners/queue/rspec_runner_spec.rb b/spec/knapsack_pro/runners/queue/rspec_runner_spec.rb index 4959d063..f672e413 100644 --- a/spec/knapsack_pro/runners/queue/rspec_runner_spec.rb +++ b/spec/knapsack_pro/runners/queue/rspec_runner_spec.rb @@ -203,7 +203,7 @@ expect(KnapsackPro::Formatters::RSpecQueueProfileFormatterExtension).to receive(:print_summary) expect(KnapsackPro::Hooks::Queue).to receive(:call_after_queue) - expect(KnapsackPro::Report).to receive(:save_node_queue_to_api).with(1) + expect(KnapsackPro::Report).to receive(:save_node_queue_to_api) expect(subject).to eq({ status: :completed, @@ -217,7 +217,7 @@ it do expect(KnapsackPro::Hooks::Queue).to receive(:call_after_queue) - expect(KnapsackPro::Report).to receive(:save_node_queue_to_api).with(0) + expect(KnapsackPro::Report).to receive(:save_node_queue_to_api) expect(subject).to eq({ status: :completed, From 18250088f687eeab76abb5d2f7fa938b795c2b8a Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Thu, 2 May 2019 18:29:17 +0200 Subject: [PATCH 2/3] Log warn instead of error for lost measured time --- lib/knapsack_pro/report.rb | 5 ++++- spec/knapsack_pro/report_spec.rb | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/lib/knapsack_pro/report.rb b/lib/knapsack_pro/report.rb index 6d46ba2b..d7be2126 100644 --- a/lib/knapsack_pro/report.rb +++ b/lib/knapsack_pro/report.rb @@ -45,7 +45,10 @@ def self.save_node_queue_to_api .select { |time_execution| time_execution != KnapsackPro::Tracker::DEFAULT_TEST_FILE_TIME } if test_files.size > 0 && measured_test_files.size == 0 - KnapsackPro.logger.error("#{test_files.size} test files were executed on this CI node but the recorded time of it was lost. Probably you have a code (i.e. RSpec hooks) that clears tmp directory in your project. Please ensure you do not remove the content of tmp/knapsack_pro/queue/ directory between tests run. Another reason might be that you forgot to add Knapsack::Adapters::RspecAdapter.bind in your rails_helper.rb or spec_helper.rb. Please follow the installation guide again: https://docs.knapsackpro.com/integration/") + KnapsackPro.logger.warn("#{test_files.size} test files were executed on this CI node but the recorded time was lost due to:") + KnapsackPro.logger.warn("1. Probably you have a code (i.e. RSpec hooks) that clears tmp directory in your project. Please ensure you do not remove the content of tmp/knapsack_pro/queue/ directory between tests run.") + KnapsackPro.logger.warn("2. Another reason might be that you forgot to add Knapsack::Adapters::RspecAdapter.bind in your rails_helper.rb or spec_helper.rb. Please follow the installation guide again: https://docs.knapsackpro.com/integration/") + KnapsackPro.logger.warn("3. All your tests are empty test files, are pending tests or have syntax error and could not be executed hence no measured time execution by knapsack_pro.") end create_build_subset(test_files) diff --git a/spec/knapsack_pro/report_spec.rb b/spec/knapsack_pro/report_spec.rb index c422f69c..6d40efd6 100644 --- a/spec/knapsack_pro/report_spec.rb +++ b/spec/knapsack_pro/report_spec.rb @@ -100,8 +100,11 @@ it 'logs error on lost info about recorded timing for test files due missing json files AND creates empty build subset' do logger = instance_double(Logger) - expect(KnapsackPro).to receive(:logger).and_return(logger) - expect(logger).to receive(:error).with('2 test files were executed on this CI node but the recorded time of it was lost. Probably you have a code (i.e. RSpec hooks) that clears tmp directory in your project. Please ensure you do not remove the content of tmp/knapsack_pro/queue/ directory between tests run. Another reason might be that you forgot to add Knapsack::Adapters::RspecAdapter.bind in your rails_helper.rb or spec_helper.rb. Please follow the installation guide again: https://docs.knapsackpro.com/integration/') + expect(KnapsackPro).to receive(:logger).exactly(4).and_return(logger) + expect(logger).to receive(:warn).with('2 test files were executed on this CI node but the recorded time was lost due to:') + expect(logger).to receive(:warn).with('1. Probably you have a code (i.e. RSpec hooks) that clears tmp directory in your project. Please ensure you do not remove the content of tmp/knapsack_pro/queue/ directory between tests run.') + expect(logger).to receive(:warn).with('2. Another reason might be that you forgot to add Knapsack::Adapters::RspecAdapter.bind in your rails_helper.rb or spec_helper.rb. Please follow the installation guide again: https://docs.knapsackpro.com/integration/') + expect(logger).to receive(:warn).with('3. All your tests are empty test files, are pending tests or have syntax error and could not be executed hence no measured time execution by knapsack_pro.') expect(described_class).to receive(:create_build_subset).with( json_test_file_a + json_test_file_b From f8231955c113495f98082442cbb1051c37f33ba1 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Thu, 2 May 2019 18:34:43 +0200 Subject: [PATCH 3/3] Update changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d48d32b..d75793e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Change Log +### 1.10.1 + +* Fix log info when measured time of tests was lost + + https://github.com/KnapsackPro/knapsack_pro-ruby/pull/85 + +https://github.com/KnapsackPro/knapsack_pro-ruby/compare/v1.10.0...v1.10.1 + ### 1.10.0 * Logs error on lost info about recorded timing for test files due to missing json files in Queue Mode