From 85b8de77493e2c455af2df91c7da7d9c1ed9f083 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Thu, 31 Oct 2024 23:37:53 +0100 Subject: [PATCH 1/9] update(CI): split by test examples above the slow test file threshold AND a single CI node --- .circleci/config.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index bca1d783..818a04a3 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -277,6 +277,17 @@ jobs: export KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true export KNAPSACK_PRO_SLOW_TEST_FILE_THRESHOLD=1 bundle exec rake knapsack_pro:queue:rspec + - run: + working_directory: ~/rails-app-with-knapsack_pro + command: | + # split by test examples above the slow test file threshold AND a single CI node || + export KNAPSACK_PRO_BRANCH="$CIRCLE_BRANCH--$CIRCLE_BUILD_NUM--queue--split-above-slow-test-file-threshold--single-node" + export KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true + export KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true + export KNAPSACK_PRO_SLOW_TEST_FILE_THRESHOLD=1 + export KNAPSACK_PRO_CI_NODE_TOTAL=1 + export KNAPSACK_PRO_CI_NODE_INDEX=0 + bundle exec rake knapsack_pro:queue:rspec - run: working_directory: ~/rails-app-with-knapsack_pro command: | From 76fb2e04c8bcfe29e0f7365ce0a6f38b017c2849 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Thu, 31 Oct 2024 23:52:13 +0100 Subject: [PATCH 2/9] revert: do not skip split by test cases on a single node --- lib/knapsack_pro/base_allocator_builder.rb | 5 -- .../base_allocator_builder_spec.rb | 54 +++++++------------ 2 files changed, 18 insertions(+), 41 deletions(-) diff --git a/lib/knapsack_pro/base_allocator_builder.rb b/lib/knapsack_pro/base_allocator_builder.rb index ed4252be..aca3fe33 100644 --- a/lib/knapsack_pro/base_allocator_builder.rb +++ b/lib/knapsack_pro/base_allocator_builder.rb @@ -36,11 +36,6 @@ def fast_and_slow_test_files_to_run test_files_to_run = all_test_files_to_run if adapter_class.split_by_test_cases_enabled? - if KnapsackPro::Config::Env.ci_node_total < 2 - KnapsackPro.logger.warn('Skipping split of test files by test cases because you are running tests on a single CI node (no parallelism)') - return test_files_to_run - end - slow_test_files = get_slow_test_files return test_files_to_run if slow_test_files.empty? diff --git a/spec/knapsack_pro/base_allocator_builder_spec.rb b/spec/knapsack_pro/base_allocator_builder_spec.rb index f6abae25..af140ea1 100644 --- a/spec/knapsack_pro/base_allocator_builder_spec.rb +++ b/spec/knapsack_pro/base_allocator_builder_spec.rb @@ -134,57 +134,39 @@ before do expect(adapter_class).to receive(:split_by_test_cases_enabled?).and_return(true) - expect(KnapsackPro::Config::Env).to receive(:ci_node_total).and_return(node_total) test_file_pattern = double expect(KnapsackPro::TestFilePattern).to receive(:call).with(adapter_class).and_return(test_file_pattern) expect(KnapsackPro::TestFileFinder).to receive(:call).with(test_file_pattern).and_return(test_files_to_run) - end - - context 'when less than 2 CI nodes' do - let(:node_total) { 1 } - it 'returns test files without test cases' do - logger = instance_double(Logger) - expect(KnapsackPro).to receive(:logger).and_return(logger) - expect(logger).to receive(:warn).with('Skipping split of test files by test cases because you are running tests on a single CI node (no parallelism)') - expect(subject).to eq test_files_to_run - end + expect(allocator_builder).to receive(:get_slow_test_files).and_return(slow_test_files) end - context 'when at least 2 CI nodes' do - let(:node_total) { 2 } - - before do - expect(allocator_builder).to receive(:get_slow_test_files).and_return(slow_test_files) + context 'when slow test files are detected' do + let(:slow_test_files) do + [ + '1_spec.rb', + '2_spec.rb', + ] end - context 'when slow test files are detected' do - let(:slow_test_files) do - [ - '1_spec.rb', - '2_spec.rb', - ] - end - - it 'returns test files with test cases' do - test_file_cases = double - expect(adapter_class).to receive(:test_file_cases_for).with(slow_test_files).and_return(test_file_cases) + it 'returns test files with test cases' do + test_file_cases = double + expect(adapter_class).to receive(:test_file_cases_for).with(slow_test_files).and_return(test_file_cases) - test_files_with_test_cases = double - expect(KnapsackPro::TestFilesWithTestCasesComposer).to receive(:call).with(test_files_to_run, slow_test_files, test_file_cases).and_return(test_files_with_test_cases) + test_files_with_test_cases = double + expect(KnapsackPro::TestFilesWithTestCasesComposer).to receive(:call).with(test_files_to_run, slow_test_files, test_file_cases).and_return(test_files_with_test_cases) - expect(subject).to eq test_files_with_test_cases - end + expect(subject).to eq test_files_with_test_cases end + end - context 'when slow test files are not detected' do - let(:slow_test_files) { [] } + context 'when slow test files are not detected' do + let(:slow_test_files) { [] } - it 'returns test files without test cases' do - expect(subject).to eq test_files_to_run - end + it 'returns test files without test cases' do + expect(subject).to eq test_files_to_run end end end From 1be0167cbf9ffb9c2d66cc3b70ae90cb547de814 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Fri, 1 Nov 2024 00:10:13 +0100 Subject: [PATCH 3/9] update(RSpec): disable split by test examples on a single node to speed up tests --- lib/knapsack_pro/config/env.rb | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/lib/knapsack_pro/config/env.rb b/lib/knapsack_pro/config/env.rb index 1c2838cd..a5744c10 100644 --- a/lib/knapsack_pro/config/env.rb +++ b/lib/knapsack_pro/config/env.rb @@ -189,7 +189,16 @@ def cucumber_queue_prefix end def rspec_split_by_test_examples - ENV.fetch('KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES', false) + return @rspec_split_by_test_examples if defined?(@rspec_split_by_test_examples) + + @rspec_split_by_test_examples = ENV.fetch('KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES', false) + + if @rspec_split_by_test_examples.to_s == 'true' && ci_node_total < 2 + KnapsackPro.logger.debug('Skipping split of test files by test cases because you are running tests on a single CI node (no parallelism)') + @rspec_split_by_test_examples = false + end + + @rspec_split_by_test_examples end def rspec_split_by_test_examples? From 10ff0d40e3f4ea5e0c8e732a6f9632da08be4877 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Fri, 1 Nov 2024 01:00:37 +0100 Subject: [PATCH 4/9] wip --- lib/knapsack_pro/config/env.rb | 24 ++++++------- spec/knapsack_pro/config/env_spec.rb | 52 ++++++++++++++++++---------- 2 files changed, 45 insertions(+), 31 deletions(-) diff --git a/lib/knapsack_pro/config/env.rb b/lib/knapsack_pro/config/env.rb index a5744c10..6c3795ac 100644 --- a/lib/knapsack_pro/config/env.rb +++ b/lib/knapsack_pro/config/env.rb @@ -188,21 +188,19 @@ def cucumber_queue_prefix ENV.fetch('KNAPSACK_PRO_CUCUMBER_QUEUE_PREFIX', 'bundle exec') end - def rspec_split_by_test_examples - return @rspec_split_by_test_examples if defined?(@rspec_split_by_test_examples) - - @rspec_split_by_test_examples = ENV.fetch('KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES', false) - - if @rspec_split_by_test_examples.to_s == 'true' && ci_node_total < 2 - KnapsackPro.logger.debug('Skipping split of test files by test cases because you are running tests on a single CI node (no parallelism)') - @rspec_split_by_test_examples = false + def rspec_split_by_test_examples? + split = ENV.fetch('KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES', false) + + if split.to_s == 'true' && ci_node_total < 2 + @logged_rspec_split_by_test_examples_message ||= + begin + KnapsackPro.logger.debug('Skipping split of test files by test examples because you are running tests on a single CI node (no parallelism)') + true + end + return false end - @rspec_split_by_test_examples - end - - def rspec_split_by_test_examples? - rspec_split_by_test_examples.to_s == 'true' + split.to_s == 'true' end def rspec_test_example_detector_prefix diff --git a/spec/knapsack_pro/config/env_spec.rb b/spec/knapsack_pro/config/env_spec.rb index b394a085..ab3fea0b 100644 --- a/spec/knapsack_pro/config/env_spec.rb +++ b/spec/knapsack_pro/config/env_spec.rb @@ -1013,37 +1013,53 @@ end end - describe '.rspec_split_by_test_examples' do - subject { described_class.rspec_split_by_test_examples } + describe '.rspec_split_by_test_examples?' do + subject { described_class.rspec_split_by_test_examples? } - context 'when ENV exists' do - before { stub_const("ENV", { 'KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES' => true }) } - it { should eq true } + context 'when KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true AND KNAPSACK_PRO_CI_NODE_TOTAL >= 2' do + before do + stub_const("ENV", { 'KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES' => 'true', 'KNAPSACK_PRO_CI_NODE_TOTAL' => '2' }) + expect(KnapsackPro).not_to receive(:logger) + end + + it { should be true } end - context "when ENV doesn't exist" do - before { stub_const("ENV", {}) } + context 'when KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=false AND KNAPSACK_PRO_CI_NODE_TOTAL >= 2' do + before do + stub_const("ENV", { 'KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES' => 'false', 'KNAPSACK_PRO_CI_NODE_TOTAL' => '2' }) + expect(KnapsackPro).not_to receive(:logger) + end + it { should be false } end - end - describe '.rspec_split_by_test_examples?' do - subject { described_class.rspec_split_by_test_examples? } + context 'when KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true AND KNAPSACK_PRO_CI_NODE_TOTAL < 2' do + before { stub_const("ENV", { 'KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES' => 'true', 'KNAPSACK_PRO_CI_NODE_TOTAL' => '1' }) } - context 'when ENV exists' do - context 'when KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true' do - before { stub_const("ENV", { 'KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES' => 'true' }) } - it { should be true } + before do + described_class.instance_variable_set(:@logged_rspec_split_by_test_examples_message, nil) end - context 'when KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=false' do - before { stub_const("ENV", { 'KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES' => 'false' }) } - it { should be false } + it { should be false } + + context 'when called twice' do + it 'logs debug message only once' do + logger = instance_double(Logger) + expect(KnapsackPro).to receive(:logger).and_return(logger) + expect(logger).to receive(:debug).with('Skipping split of test files by test examples because you are running tests on a single CI node (no parallelism)') + + 2.times { described_class.rspec_split_by_test_examples? } + end end end context "when ENV doesn't exist" do - before { stub_const("ENV", {}) } + before do + stub_const("ENV", {}) + expect(KnapsackPro).not_to receive(:logger) + end + it { should be false } end end From 31ad630357742f7e1a549704f0005deacca11f22 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Fri, 1 Nov 2024 01:01:38 +0100 Subject: [PATCH 5/9] Update env.rb --- lib/knapsack_pro/config/env.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/knapsack_pro/config/env.rb b/lib/knapsack_pro/config/env.rb index 6c3795ac..6b13aee0 100644 --- a/lib/knapsack_pro/config/env.rb +++ b/lib/knapsack_pro/config/env.rb @@ -189,9 +189,9 @@ def cucumber_queue_prefix end def rspec_split_by_test_examples? - split = ENV.fetch('KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES', false) + split = ENV.fetch('KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES', false).to_s == 'true' - if split.to_s == 'true' && ci_node_total < 2 + if split && ci_node_total < 2 @logged_rspec_split_by_test_examples_message ||= begin KnapsackPro.logger.debug('Skipping split of test files by test examples because you are running tests on a single CI node (no parallelism)') @@ -200,7 +200,7 @@ def rspec_split_by_test_examples? return false end - split.to_s == 'true' + split end def rspec_test_example_detector_prefix From 9efed0df4130636980b4476efef376e392e4e2fa Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Fri, 1 Nov 2024 01:24:02 +0100 Subject: [PATCH 6/9] add memoization to avoid calling ci_node_total many times which prints additional log messages --- lib/knapsack_pro/config/env.rb | 14 ++++++-------- spec/knapsack_pro/config/env_spec.rb | 9 +++++---- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/lib/knapsack_pro/config/env.rb b/lib/knapsack_pro/config/env.rb index 6b13aee0..01e5c4ee 100644 --- a/lib/knapsack_pro/config/env.rb +++ b/lib/knapsack_pro/config/env.rb @@ -189,18 +189,16 @@ def cucumber_queue_prefix end def rspec_split_by_test_examples? + return @rspec_split_by_test_examples if defined?(@rspec_split_by_test_examples) + split = ENV.fetch('KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES', false).to_s == 'true' if split && ci_node_total < 2 - @logged_rspec_split_by_test_examples_message ||= - begin - KnapsackPro.logger.debug('Skipping split of test files by test examples because you are running tests on a single CI node (no parallelism)') - true - end - return false + KnapsackPro.logger.debug('Skipping split of test files by test examples because you are running tests on a single CI node (no parallelism)') + @rspec_split_by_test_examples = false + else + @rspec_split_by_test_examples = split end - - split end def rspec_test_example_detector_prefix diff --git a/spec/knapsack_pro/config/env_spec.rb b/spec/knapsack_pro/config/env_spec.rb index ab3fea0b..808e7b72 100644 --- a/spec/knapsack_pro/config/env_spec.rb +++ b/spec/knapsack_pro/config/env_spec.rb @@ -1016,6 +1016,11 @@ describe '.rspec_split_by_test_examples?' do subject { described_class.rspec_split_by_test_examples? } + before do + described_class.remove_instance_variable(:@rspec_split_by_test_examples) + rescue NameError + end + context 'when KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true AND KNAPSACK_PRO_CI_NODE_TOTAL >= 2' do before do stub_const("ENV", { 'KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES' => 'true', 'KNAPSACK_PRO_CI_NODE_TOTAL' => '2' }) @@ -1037,10 +1042,6 @@ context 'when KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true AND KNAPSACK_PRO_CI_NODE_TOTAL < 2' do before { stub_const("ENV", { 'KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES' => 'true', 'KNAPSACK_PRO_CI_NODE_TOTAL' => '1' }) } - before do - described_class.instance_variable_set(:@logged_rspec_split_by_test_examples_message, nil) - end - it { should be false } context 'when called twice' do From f2d62d9f927a0a9f0501b381e53387065227ae23 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Fri, 1 Nov 2024 01:27:03 +0100 Subject: [PATCH 7/9] Update CHANGELOG.md --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index acaadce6..729a17d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +### 7.12.1 + +* fix(RSpec split by examples): properly disable split by test examples on a single node to speed up tests + + https://github.com/KnapsackPro/knapsack_pro-ruby/pull/283 + +https://github.com/KnapsackPro/knapsack_pro-ruby/compare/v7.12.0...v7.12.1 + ### 7.12.0 * Add `KNAPSACK_PRO_SLOW_TEST_FILE_THRESHOLD` to improve the RSpec split by examples feature with many skipped tests From 269d5fa9d3eb92c0fc8d10a221f1754120f38137 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Fri, 1 Nov 2024 01:29:04 +0100 Subject: [PATCH 8/9] Update env_spec.rb --- spec/knapsack_pro/config/env_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/knapsack_pro/config/env_spec.rb b/spec/knapsack_pro/config/env_spec.rb index 808e7b72..af4b21dc 100644 --- a/spec/knapsack_pro/config/env_spec.rb +++ b/spec/knapsack_pro/config/env_spec.rb @@ -1045,7 +1045,7 @@ it { should be false } context 'when called twice' do - it 'logs debug message only once' do + it 'logs a debug message only once' do logger = instance_double(Logger) expect(KnapsackPro).to receive(:logger).and_return(logger) expect(logger).to receive(:debug).with('Skipping split of test files by test examples because you are running tests on a single CI node (no parallelism)') From 7bb461731698a55b9df456a63634504c99c723c1 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Fri, 1 Nov 2024 01:34:45 +0100 Subject: [PATCH 9/9] it's enough to test split on a single node --- .circleci/config.yml | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 818a04a3..c18e968d 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -280,11 +280,10 @@ jobs: - run: working_directory: ~/rails-app-with-knapsack_pro command: | - # split by test examples above the slow test file threshold AND a single CI node || - export KNAPSACK_PRO_BRANCH="$CIRCLE_BRANCH--$CIRCLE_BUILD_NUM--queue--split-above-slow-test-file-threshold--single-node" + # split by test examples AND a single CI node || + export KNAPSACK_PRO_BRANCH="$CIRCLE_BRANCH--$CIRCLE_BUILD_NUM--queue--split--single-node" export KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true export KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true - export KNAPSACK_PRO_SLOW_TEST_FILE_THRESHOLD=1 export KNAPSACK_PRO_CI_NODE_TOTAL=1 export KNAPSACK_PRO_CI_NODE_INDEX=0 bundle exec rake knapsack_pro:queue:rspec