fix(RSpec split by examples): properly disable split by test examples…

… on a single node to speed up tests (#283)
KnapsackPro · Nov 1, 2024 · 180db3f · 180db3f
1 parent a780923
commit 180db3f
Show file tree

Hide file tree

Showing 6 changed files with 82 additions and 65 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -277,6 +277,16 @@ 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 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_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: |

diff --git 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

diff --git 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/lib/knapsack_pro/config/env.rb b/lib/knapsack_pro/config/env.rb
@@ -188,12 +188,17 @@ def cucumber_queue_prefix
           ENV.fetch('KNAPSACK_PRO_CUCUMBER_QUEUE_PREFIX', 'bundle exec')
         end
 
-        def rspec_split_by_test_examples
-          ENV.fetch('KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES', false)
-        end
-
         def rspec_split_by_test_examples?
-          rspec_split_by_test_examples.to_s == 'true'
+          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
+            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
         end
 
         def rspec_test_example_detector_prefix

diff --git 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

diff --git a/spec/knapsack_pro/config/env_spec.rb b/spec/knapsack_pro/config/env_spec.rb
@@ -1013,37 +1013,54 @@
     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 }
+    before do
+      described_class.remove_instance_variable(:@rspec_split_by_test_examples)
+    rescue NameError
     end
 
-    context "when ENV doesn't exist" do
-      before { stub_const("ENV", {}) }
+    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 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 }
-      end
+      it { should be false }
 
-      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 }
+      context 'when called twice' 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)')
+
+          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