From 95f0d343fc06d2f0d1fad64ead7b574216c54fb5 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Wed, 30 Oct 2024 12:33:58 +0100 Subject: [PATCH 1/4] Update rspec_merger_spec.rb --- .../test_case_mergers/rspec_merger_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb b/spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb index 2d9088f9..745b4a88 100644 --- a/spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb +++ b/spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb @@ -2,7 +2,7 @@ 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 }, @@ -10,7 +10,7 @@ ] 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,7 +28,7 @@ ] 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 }, @@ -36,11 +36,11 @@ 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 + context 'when test files have test example paths and the full test file path exists simultaneously' 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 + # full test file path exists alongside 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 }, @@ -48,7 +48,7 @@ ] end - it 'returns merged paths for test examples and sum of their time_execution' do + it 'merges the 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' => 4.0 }, From 6207b0d103f29fded027d52dfea9039a4b035d60 Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Wed, 30 Oct 2024 12:42:58 +0100 Subject: [PATCH 2/4] Update rspec_merger_spec.rb --- .../test_case_mergers/rspec_merger_spec.rb | 50 +++++++++++++------ 1 file changed, 36 insertions(+), 14 deletions(-) diff --git a/spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb b/spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb index 745b4a88..e6ba7bb3 100644 --- a/spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb +++ b/spec/knapsack_pro/test_case_mergers/rspec_merger_spec.rb @@ -37,22 +37,44 @@ end context 'when test files have test example paths and the full test file path exists simultaneously' 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' => 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 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 'merges the 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' => 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 From 89b81b6ee47ac4c96b7f5c799ab6001434496b3e Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Wed, 30 Oct 2024 13:10:54 +0100 Subject: [PATCH 3/4] Update rspec_merger.rb --- .../test_case_mergers/rspec_merger.rb | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/lib/knapsack_pro/test_case_mergers/rspec_merger.rb b/lib/knapsack_pro/test_case_mergers/rspec_merger.rb index f90962fa..5f3e9454 100644 --- a/lib/knapsack_pro/test_case_mergers/rspec_merger.rb +++ b/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 From a43db9991cadf69ac54dc08ddc13de3d8446973e Mon Sep 17 00:00:00 2001 From: Artur Trzop Date: Wed, 30 Oct 2024 13:23:17 +0100 Subject: [PATCH 4/4] Update CHANGELOG.md --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36559b71..f9f81c00 100644 --- a/CHANGELOG.md +++ b/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.