fix(RSpec split by examples): Properly determine slow test files when test example execution times and full test file execution time are both known

CHANGELOG.md

@@ -1,5 +1,13 @@
 # Changelog
 
+### 7.11.0
+
+* fix(RSpec split by examples): Properly determine slow test files when test example execution times and full test file execution time are both known
+
+    https://github.com/KnapsackPro/knapsack_pro-ruby/pull/281
+
+https://github.com/KnapsackPro/knapsack_pro-ruby/compare/v7.10.0...v7.11.0
+
 ### 7.10.0
 
 * Improve the RSpec split by examples feature. Use test file execution times for existing test files on the disk to determine slow test files. This fixes issue with detecting slow test files when API token is shared between multiple test suites.

lib/knapsack_pro/test_case_mergers/rspec_merger.rb

@@ -4,17 +4,27 @@ module KnapsackPro
   module TestCaseMergers
     class RSpecMerger < BaseMerger
       def call
-        merged_test_files_hash = {}
+        all_test_files_hash = {}
+        merged_test_file_examples_hash = {}
+
         test_files.each do |test_file|
-          test_file_path = extract_test_file_path(test_file.fetch('path'))
+          path = test_file.fetch('path')
+          test_file_path = extract_test_file_path(path)
+
+          if rspec_id_path?(path)
+            merged_test_file_examples_hash[test_file_path] ||= 0.0
+            merged_test_file_examples_hash[test_file_path] += test_file.fetch('time_execution')
+          else
+            all_test_files_hash[test_file_path] = test_file.fetch('time_execution')
+          end
+        end
 
-          # must be float (default type for time execution from API)
-          merged_test_files_hash[test_file_path] ||= 0.0
-          merged_test_files_hash[test_file_path] += test_file.fetch('time_execution')
+        merged_test_file_examples_hash.each do |path, time_execution|
+          all_test_files_hash[path] = [time_execution, all_test_files_hash[path]].compact.max
         end
 
         merged_test_files = []
-        merged_test_files_hash.each do |path, time_execution|
+        all_test_files_hash.each do |path, time_execution|
           merged_test_files << {
             'path' => path,
             'time_execution' => time_execution
@@ -31,6 +41,12 @@ def call
       def extract_test_file_path(path)
         path.gsub(/\.rb\[.+\]$/, '.rb')
       end
+
+      def rspec_id_path?(path)
+        path_with_id_regex = /.+_spec\.rb\[.+\]$/
+
+        path&.match?(path_with_id_regex)
+      end
     end
   end
 end

spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb

@@ -2,15 +2,15 @@
   describe '#call' do
     subject { described_class.new(test_files).call }
 
-    context 'when all test files are not test example paths' do
+    context 'when test files are regular file paths (not test example paths)' do
       let(:test_files) do
         [
           { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
           { 'path' => 'spec/b_spec.rb', 'time_execution' => 2.2 },
         ]
       end
 
-      it do
+      it 'returns the test files unchanged' do
         expect(subject).to eq([
           { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
           { 'path' => 'spec/b_spec.rb', 'time_execution' => 2.2 },
@@ -28,31 +28,53 @@
         ]
       end
 
-      it 'returns merged paths for test examples and sum of their time_execution' do
+      it 'merges the test example paths and sums their execution times' do
         expect(subject).to eq([
           { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
           { 'path' => 'spec/test_case_spec.rb', 'time_execution' => 3.0 },
         ])
       end
     end
 
-    context 'when test files have test example paths and at the same time test file path for test example path is present as full test file path' do
-      let(:test_files) do
-        [
-          { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
-          # full test file path is present despite existing test example paths
-          { 'path' => 'spec/test_case_spec.rb', 'time_execution' => 1.0 },
-          # test example paths
-          { 'path' => 'spec/test_case_spec.rb[1:1]', 'time_execution' => 2.2 },
-          { 'path' => 'spec/test_case_spec.rb[1:2]', 'time_execution' => 0.8 },
-        ]
+    context 'when test files have test example paths and the full test file path exists simultaneously' do
+      context 'when the full test file path has a higher execution time than the sum of test example paths' do
+        let(:test_files) do
+          [
+            { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
+            # full test file path exists alongside test example paths
+            { 'path' => 'spec/test_case_spec.rb', 'time_execution' => 3.1 },
+            # test example paths
+            { 'path' => 'spec/test_case_spec.rb[1:1]', 'time_execution' => 2.2 },
+            { 'path' => 'spec/test_case_spec.rb[1:2]', 'time_execution' => 0.8 },
+          ]
+        end
+
+        it 'returns the full test file path execution time' do
+          expect(subject).to eq([
+            { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
+            { 'path' => 'spec/test_case_spec.rb', 'time_execution' => 3.1 },
+          ])
+        end
       end
 
-      it 'returns merged paths for test examples and sum of their time_execution' do
-        expect(subject).to eq([
-          { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
-          { 'path' => 'spec/test_case_spec.rb', 'time_execution' => 4.0 },
-        ])
+      context 'when the full test file path has a lower execution time than the sum of test example paths' do
+        let(:test_files) do
+          [
+            { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
+            # full test file path exists alongside test example paths
+            { 'path' => 'spec/test_case_spec.rb', 'time_execution' => 2.9 },
+            # test example paths
+            { 'path' => 'spec/test_case_spec.rb[1:1]', 'time_execution' => 2.2 },
+            { 'path' => 'spec/test_case_spec.rb[1:2]', 'time_execution' => 0.8 },
+          ]
+        end
+
+        it "returns the sum of test example paths' execution times" do
+          expect(subject).to eq([
+            { 'path' => 'spec/a_spec.rb', 'time_execution' => 1.1 },
+            { 'path' => 'spec/test_case_spec.rb', 'time_execution' => 3.0 },
+          ])
+        end
       end
     end
   end