diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c79b491..12bde698 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Change Log +### 2.10.0 + +* Add support for an attempt to connect to existing Queue on API side to reduce slow requests number + + https://github.com/KnapsackPro/knapsack_pro-ruby/pull/133 + +https://github.com/KnapsackPro/knapsack_pro-ruby/compare/v2.9.0...v2.10.0 + ### 2.9.0 * Use `Process.clock_gettime` to measure track execution time diff --git a/lib/knapsack_pro/base_allocator_builder.rb b/lib/knapsack_pro/base_allocator_builder.rb index 0ba0a66f..657e0047 100644 --- a/lib/knapsack_pro/base_allocator_builder.rb +++ b/lib/knapsack_pro/base_allocator_builder.rb @@ -28,6 +28,10 @@ def fallback_mode_test_files all_test_files_to_run end + def lazy_fallback_mode_test_files + lambda { fallback_mode_test_files } + end + # detect test files present on the disk that should be run # this may include some fast test files + slow test files split by test cases def fast_and_slow_test_files_to_run @@ -63,6 +67,10 @@ def fast_and_slow_test_files_to_run end end + def lazy_fast_and_slow_test_files_to_run + lambda { fast_and_slow_test_files_to_run } + end + private attr_reader :adapter_class diff --git a/lib/knapsack_pro/client/api/v1/queues.rb b/lib/knapsack_pro/client/api/v1/queues.rb index cafda967..55af1b3e 100644 --- a/lib/knapsack_pro/client/api/v1/queues.rb +++ b/lib/knapsack_pro/client/api/v1/queues.rb @@ -3,11 +3,14 @@ module Client module API module V1 class Queues < Base + CODE_ATTEMPT_CONNECT_TO_QUEUE_FAILED = 'ATTEMPT_CONNECT_TO_QUEUE_FAILED' + class << self def queue(args) request_hash = { :fixed_queue_split => KnapsackPro::Config::Env.fixed_queue_split, :can_initialize_queue => args.fetch(:can_initialize_queue), + :attempt_connect_to_queue => args.fetch(:attempt_connect_to_queue), :commit_hash => args.fetch(:commit_hash), :branch => args.fetch(:branch), :node_total => args.fetch(:node_total), @@ -15,7 +18,7 @@ def queue(args) :node_build_id => KnapsackPro::Config::Env.ci_node_build_id, } - if request_hash[:can_initialize_queue] + if request_hash[:can_initialize_queue] && !request_hash[:attempt_connect_to_queue] request_hash.merge!({ :test_files => args.fetch(:test_files) }) diff --git a/lib/knapsack_pro/client/connection.rb b/lib/knapsack_pro/client/connection.rb index 38315e5a..0b7e55d4 100644 --- a/lib/knapsack_pro/client/connection.rb +++ b/lib/knapsack_pro/client/connection.rb @@ -25,6 +25,11 @@ def errors? !!(response_body && (response_body['errors'] || response_body['error'])) end + def api_code + return unless response_body + response_body['code'] + end + def server_error? status = http_response.code.to_i status >= 500 && status < 600 diff --git a/lib/knapsack_pro/queue_allocator.rb b/lib/knapsack_pro/queue_allocator.rb index dbc0ac30..b66bb175 100644 --- a/lib/knapsack_pro/queue_allocator.rb +++ b/lib/knapsack_pro/queue_allocator.rb @@ -1,8 +1,8 @@ module KnapsackPro class QueueAllocator def initialize(args) - @fast_and_slow_test_files_to_run = args.fetch(:fast_and_slow_test_files_to_run) - @fallback_mode_test_files = args.fetch(:fallback_mode_test_files) + @lazy_fast_and_slow_test_files_to_run = args.fetch(:lazy_fast_and_slow_test_files_to_run) + @lazy_fallback_mode_test_files = args.fetch(:lazy_fallback_mode_test_files) @ci_node_total = args.fetch(:ci_node_total) @ci_node_index = args.fetch(:ci_node_index) @ci_node_build_id = args.fetch(:ci_node_build_id) @@ -11,9 +11,18 @@ def initialize(args) def test_file_paths(can_initialize_queue, executed_test_files) return [] if @fallback_activated - action = build_action(can_initialize_queue) + action = build_action(can_initialize_queue, attempt_connect_to_queue: can_initialize_queue) connection = KnapsackPro::Client::Connection.new(action) response = connection.call + + # when attempt to connect to existing queue on API side failed because queue does not exist yet + if can_initialize_queue && connection.success? && connection.api_code == KnapsackPro::Client::API::V1::Queues::CODE_ATTEMPT_CONNECT_TO_QUEUE_FAILED + # make attempt to initalize a new queue on API side + action = build_action(can_initialize_queue, attempt_connect_to_queue: false) + connection = KnapsackPro::Client::Connection.new(action) + response = connection.call + end + if connection.success? raise ArgumentError.new(response) if connection.errors? prepare_test_files(response) @@ -37,40 +46,58 @@ def test_file_paths(can_initialize_queue, executed_test_files) private - attr_reader :fast_and_slow_test_files_to_run, - :fallback_mode_test_files, + attr_reader :lazy_fast_and_slow_test_files_to_run, + :lazy_fallback_mode_test_files, :ci_node_total, :ci_node_index, :ci_node_build_id, :repository_adapter + # This method might be slow because it reads test files from disk. + # This method can be very slow (a few seconds or more) when you use RSpec split by test examples feature because RSpec needs to generate JSON report with test examples ids + def lazy_loaded_fast_and_slow_test_files_to_run + @lazy_loaded_fast_and_slow_test_files_to_run ||= lazy_fast_and_slow_test_files_to_run.call + end + def encrypted_test_files - KnapsackPro::Crypto::Encryptor.call(fast_and_slow_test_files_to_run) + KnapsackPro::Crypto::Encryptor.call(lazy_loaded_fast_and_slow_test_files_to_run) end def encrypted_branch KnapsackPro::Crypto::BranchEncryptor.call(repository_adapter.branch) end - def build_action(can_initialize_queue) + def build_action(can_initialize_queue, attempt_connect_to_queue:) + # read test files from disk only when needed because it can be slow operation + test_files = + if can_initialize_queue && !attempt_connect_to_queue + encrypted_test_files + end + KnapsackPro::Client::API::V1::Queues.queue( can_initialize_queue: can_initialize_queue, + attempt_connect_to_queue: attempt_connect_to_queue, commit_hash: repository_adapter.commit_hash, branch: encrypted_branch, node_total: ci_node_total, node_index: ci_node_index, node_build_id: ci_node_build_id, - test_files: encrypted_test_files, + test_files: test_files, ) end def prepare_test_files(response) - decrypted_test_files = KnapsackPro::Crypto::Decryptor.call(fast_and_slow_test_files_to_run, response['test_files']) - KnapsackPro::TestFilePresenter.paths(decrypted_test_files) + # when encryption is disabled we can avoid calling slow method lazy_loaded_fast_and_slow_test_files_to_run + if KnapsackPro::Config::Env.test_files_encrypted? + decrypted_test_files = KnapsackPro::Crypto::Decryptor.call(lazy_loaded_fast_and_slow_test_files_to_run, response['test_files']) + KnapsackPro::TestFilePresenter.paths(decrypted_test_files) + else + KnapsackPro::TestFilePresenter.paths(response['test_files']) + end end def fallback_test_files(executed_test_files) - test_flat_distributor = KnapsackPro::TestFlatDistributor.new(fallback_mode_test_files, ci_node_total) + test_flat_distributor = KnapsackPro::TestFlatDistributor.new(lazy_fallback_mode_test_files.call, ci_node_total) test_files_for_node_index = test_flat_distributor.test_files_for_node(ci_node_index) KnapsackPro::TestFilePresenter.paths(test_files_for_node_index) - executed_test_files end diff --git a/lib/knapsack_pro/queue_allocator_builder.rb b/lib/knapsack_pro/queue_allocator_builder.rb index f146600a..5aacc03b 100644 --- a/lib/knapsack_pro/queue_allocator_builder.rb +++ b/lib/knapsack_pro/queue_allocator_builder.rb @@ -2,8 +2,8 @@ module KnapsackPro class QueueAllocatorBuilder < BaseAllocatorBuilder def allocator KnapsackPro::QueueAllocator.new( - fast_and_slow_test_files_to_run: fast_and_slow_test_files_to_run, - fallback_mode_test_files: fallback_mode_test_files, + lazy_fast_and_slow_test_files_to_run: lazy_fast_and_slow_test_files_to_run, + lazy_fallback_mode_test_files: lazy_fallback_mode_test_files, ci_node_total: env.ci_node_total, ci_node_index: env.ci_node_index, ci_node_build_id: env.ci_node_build_id, diff --git a/spec/knapsack_pro/client/api/v1/queues_spec.rb b/spec/knapsack_pro/client/api/v1/queues_spec.rb index 0106c801..e983fabd 100644 --- a/spec/knapsack_pro/client/api/v1/queues_spec.rb +++ b/spec/knapsack_pro/client/api/v1/queues_spec.rb @@ -11,6 +11,7 @@ subject do described_class.queue( can_initialize_queue: can_initialize_queue, + attempt_connect_to_queue: attempt_connect_to_queue, commit_hash: commit_hash, branch: branch, node_total: node_total, @@ -24,8 +25,33 @@ expect(KnapsackPro::Config::Env).to receive(:ci_node_build_id).and_return(node_build_id) end - context 'when can_initialize_queue=true' do + context 'when can_initialize_queue=true and attempt_connect_to_queue=true' do let(:can_initialize_queue) { true } + let(:attempt_connect_to_queue) { true } + + it 'does not send test_files among other params' do + action = double + expect(KnapsackPro::Client::API::Action).to receive(:new).with({ + endpoint_path: '/v1/queues/queue', + http_method: :post, + request_hash: { + fixed_queue_split: fixed_queue_split, + can_initialize_queue: can_initialize_queue, + attempt_connect_to_queue: attempt_connect_to_queue, + commit_hash: commit_hash, + branch: branch, + node_total: node_total, + node_index: node_index, + node_build_id: node_build_id, + } + }).and_return(action) + expect(subject).to eq action + end + end + + context 'when can_initialize_queue=true and attempt_connect_to_queue=false' do + let(:can_initialize_queue) { true } + let(:attempt_connect_to_queue) { false } it 'sends test_files among other params' do action = double @@ -35,6 +61,7 @@ request_hash: { fixed_queue_split: fixed_queue_split, can_initialize_queue: can_initialize_queue, + attempt_connect_to_queue: attempt_connect_to_queue, commit_hash: commit_hash, branch: branch, node_total: node_total, @@ -47,8 +74,9 @@ end end - context 'when can_initialize_queue=false' do + context 'when can_initialize_queue=false and attempt_connect_to_queue=false' do let(:can_initialize_queue) { false } + let(:attempt_connect_to_queue) { false } it 'does not send test_files among other params' do action = double @@ -58,6 +86,7 @@ request_hash: { fixed_queue_split: fixed_queue_split, can_initialize_queue: can_initialize_queue, + attempt_connect_to_queue: attempt_connect_to_queue, commit_hash: commit_hash, branch: branch, node_total: node_total, diff --git a/spec/knapsack_pro/queue_allocator_builder_spec.rb b/spec/knapsack_pro/queue_allocator_builder_spec.rb index 92f6edf5..8bb8128e 100644 --- a/spec/knapsack_pro/queue_allocator_builder_spec.rb +++ b/spec/knapsack_pro/queue_allocator_builder_spec.rb @@ -8,11 +8,11 @@ subject { allocator_builder.allocator } before do - fast_and_slow_test_files_to_run = double - expect(allocator_builder).to receive(:fast_and_slow_test_files_to_run).and_return(fast_and_slow_test_files_to_run) + lazy_fast_and_slow_test_files_to_run = double + expect(allocator_builder).to receive(:lazy_fast_and_slow_test_files_to_run).and_return(lazy_fast_and_slow_test_files_to_run) - fallback_mode_test_files = double - expect(allocator_builder).to receive(:fallback_mode_test_files).and_return(fallback_mode_test_files) + lazy_fallback_mode_test_files = double + expect(allocator_builder).to receive(:lazy_fallback_mode_test_files).and_return(lazy_fallback_mode_test_files) repository_adapter = double expect(KnapsackPro::RepositoryAdapterInitiator).to receive(:call).and_return(repository_adapter) @@ -25,8 +25,8 @@ expect(KnapsackPro::Config::Env).to receive(:ci_node_build_id).and_return(ci_node_build_id) expect(KnapsackPro::QueueAllocator).to receive(:new).with( - fast_and_slow_test_files_to_run: fast_and_slow_test_files_to_run, - fallback_mode_test_files: fallback_mode_test_files, + lazy_fast_and_slow_test_files_to_run: lazy_fast_and_slow_test_files_to_run, + lazy_fallback_mode_test_files: lazy_fallback_mode_test_files, ci_node_total: ci_node_total, ci_node_index: ci_node_index, ci_node_build_id: ci_node_build_id, diff --git a/spec/knapsack_pro/queue_allocator_spec.rb b/spec/knapsack_pro/queue_allocator_spec.rb index 1f218d01..7e069166 100644 --- a/spec/knapsack_pro/queue_allocator_spec.rb +++ b/spec/knapsack_pro/queue_allocator_spec.rb @@ -1,6 +1,8 @@ describe KnapsackPro::QueueAllocator do - let(:fast_and_slow_test_files_to_run) { double } - let(:fallback_mode_test_files) { double } + let(:lazy_loaded_fast_and_slow_test_files_to_run) { double } + let(:lazy_fast_and_slow_test_files_to_run) { double(call: lazy_loaded_fast_and_slow_test_files_to_run) } + let(:lazy_loaded_fallback_mode_test_files) { double } + let(:lazy_fallback_mode_test_files) { double(call: lazy_loaded_fallback_mode_test_files) } let(:ci_node_total) { double } let(:ci_node_index) { double } let(:ci_node_build_id) { double } @@ -8,8 +10,8 @@ let(:queue_allocator) do described_class.new( - fast_and_slow_test_files_to_run: fast_and_slow_test_files_to_run, - fallback_mode_test_files: fallback_mode_test_files, + lazy_fast_and_slow_test_files_to_run: lazy_fast_and_slow_test_files_to_run, + lazy_fallback_mode_test_files: lazy_fallback_mode_test_files, ci_node_total: ci_node_total, ci_node_index: ci_node_index, ci_node_build_id: ci_node_build_id, @@ -18,71 +20,13 @@ end describe '#test_file_paths' do - let(:can_initialize_queue) { double } let(:executed_test_files) { [] } let(:response) { double } + let(:api_code) { nil } subject { queue_allocator.test_file_paths(can_initialize_queue, executed_test_files) } - before do - encrypted_test_files = double - expect(KnapsackPro::Crypto::Encryptor).to receive(:call).with(fast_and_slow_test_files_to_run).and_return(encrypted_test_files) - - encrypted_branch = double - expect(KnapsackPro::Crypto::BranchEncryptor).to receive(:call).with(repository_adapter.branch).and_return(encrypted_branch) - - action = double - expect(KnapsackPro::Client::API::V1::Queues).to receive(:queue).with( - can_initialize_queue: can_initialize_queue, - commit_hash: repository_adapter.commit_hash, - branch: encrypted_branch, - node_total: ci_node_total, - node_index: ci_node_index, - node_build_id: ci_node_build_id, - test_files: encrypted_test_files, - ).and_return(action) - - connection = instance_double(KnapsackPro::Client::Connection, - call: response, - success?: success?, - errors?: errors?) - expect(KnapsackPro::Client::Connection).to receive(:new).with(action).and_return(connection) - end - - context 'when successful request to API' do - let(:success?) { true } - - context 'when response has errors' do - let(:errors?) { true } - - it do - expect { subject }.to raise_error(ArgumentError) - end - end - - context 'when response has no errors' do - let(:errors?) { false } - let(:response) do - { - 'test_files' => [ - { 'path' => 'a_spec.rb' }, - { 'path' => 'b_spec.rb' }, - ] - } - end - - before do - expect(KnapsackPro::Crypto::Decryptor).to receive(:call).with(fast_and_slow_test_files_to_run, response['test_files']).and_call_original - end - - it { should eq ['a_spec.rb', 'b_spec.rb'] } - end - end - - context 'when not successful request to API' do - let(:success?) { false } - let(:errors?) { false } - + shared_examples_for 'when connection to API failed (fallback mode)' do context 'when fallback mode is disabled' do before do expect(KnapsackPro::Config::Env).to receive(:fallback_mode_enabled?).and_return(false) @@ -122,7 +66,7 @@ context 'when fallback mode started' do before do test_flat_distributor = instance_double(KnapsackPro::TestFlatDistributor) - expect(KnapsackPro::TestFlatDistributor).to receive(:new).with(fallback_mode_test_files, ci_node_total).and_return(test_flat_distributor) + expect(KnapsackPro::TestFlatDistributor).to receive(:new).with(lazy_loaded_fallback_mode_test_files, ci_node_total).and_return(test_flat_distributor) expect(test_flat_distributor).to receive(:test_files_for_node).with(ci_node_index).and_return([ { 'path' => 'c_spec.rb' }, { 'path' => 'd_spec.rb' }, @@ -146,5 +90,256 @@ end end end + + context 'when can_initialize_queue=true' do + let(:can_initialize_queue) { true } + + before do + encrypted_branch = double + expect(KnapsackPro::Crypto::BranchEncryptor).to receive(:call).with(repository_adapter.branch).and_return(encrypted_branch) + + action = double + expect(KnapsackPro::Client::API::V1::Queues).to receive(:queue).with( + can_initialize_queue: can_initialize_queue, + attempt_connect_to_queue: true, # when can_initialize_queue=true then expect attempt_connect_to_queue=true + commit_hash: repository_adapter.commit_hash, + branch: encrypted_branch, + node_total: ci_node_total, + node_index: ci_node_index, + node_build_id: ci_node_build_id, + test_files: nil, # when attempt_connect_to_queue=true then expect test_files is nil to make fast request to API + ).and_return(action) + + connection = instance_double(KnapsackPro::Client::Connection, + call: response, + success?: success?, + errors?: errors?, + api_code: api_code) + expect(KnapsackPro::Client::Connection).to receive(:new).with(action).and_return(connection) + end + + context 'when successful request to API' do + let(:success?) { true } + + context 'when response has errors' do + let(:errors?) { true } + + it do + expect { subject }.to raise_error(ArgumentError) + end + end + + context 'when response has no errors' do + let(:errors?) { false } + + context 'when response returns test files (successful attempt to connect to queue already existing on the API side)' do + let(:test_files) do + [ + { 'path' => 'a_spec.rb' }, + { 'path' => 'b_spec.rb' }, + ] + end + let(:response) do + { 'test_files' => test_files } + end + + context 'when test files encryption is enabled' do + before do + expect(KnapsackPro::Config::Env).to receive(:test_files_encrypted?).and_return(true) + expect(KnapsackPro::Crypto::Decryptor).to receive(:call).with(lazy_loaded_fast_and_slow_test_files_to_run, response['test_files']).and_return(test_files) + end + + it { should eq ['a_spec.rb', 'b_spec.rb'] } + end + + context 'when test files encryption is disabled' do + before do + expect(KnapsackPro::Config::Env).to receive(:test_files_encrypted?).and_return(false) + end + + it { should eq ['a_spec.rb', 'b_spec.rb'] } + end + end + + context 'when response has code=ATTEMPT_CONNECT_TO_QUEUE_FAILED' do + let(:response) do + { 'code' => 'ATTEMPT_CONNECT_TO_QUEUE_FAILED' } + end + let(:api_code) { 'ATTEMPT_CONNECT_TO_QUEUE_FAILED' } + + before do + encrypted_branch = double + expect(KnapsackPro::Crypto::BranchEncryptor).to receive(:call).with(repository_adapter.branch).and_return(encrypted_branch) + + encrypted_test_files = double + expect(KnapsackPro::Crypto::Encryptor).to receive(:call).with(lazy_loaded_fast_and_slow_test_files_to_run).and_return(encrypted_test_files) + + # 2nd request is no more an attempt to connect to queue. + # We want to try to initalize a new queue so we will also send list of test files from disk. + action = double + expect(KnapsackPro::Client::API::V1::Queues).to receive(:queue).with( + can_initialize_queue: can_initialize_queue, + attempt_connect_to_queue: false, + commit_hash: repository_adapter.commit_hash, + branch: encrypted_branch, + node_total: ci_node_total, + node_index: ci_node_index, + node_build_id: ci_node_build_id, + test_files: encrypted_test_files, + ).and_return(action) + + connection = instance_double(KnapsackPro::Client::Connection, + call: response2, + success?: response2_success?, + errors?: response2_errors?, + api_code: nil) + expect(KnapsackPro::Client::Connection).to receive(:new).with(action).and_return(connection) + end + + context 'when successful 2nd request to API' do + let(:response2_success?) { true } + + context 'when 2nd response has errors' do + let(:response2_errors?) { true } + let(:response2) { nil } + + it do + expect { subject }.to raise_error(ArgumentError) + end + end + + context 'when 2nd response has no errors' do + let(:response2_errors?) { false } + + context 'when 2nd response returns test files (successful attempt to connect to queue already existing on the API side)' do + let(:test_files) do + [ + { 'path' => 'a_spec.rb' }, + { 'path' => 'b_spec.rb' }, + ] + end + let(:response2) do + { 'test_files' => test_files } + end + + context 'when test files encryption is enabled' do + before do + expect(KnapsackPro::Config::Env).to receive(:test_files_encrypted?).and_return(true) + expect(KnapsackPro::Crypto::Decryptor).to receive(:call).with(lazy_loaded_fast_and_slow_test_files_to_run, response2['test_files']).and_return(test_files) + end + + it { should eq ['a_spec.rb', 'b_spec.rb'] } + end + + context 'when test files encryption is disabled' do + before do + expect(KnapsackPro::Config::Env).to receive(:test_files_encrypted?).and_return(false) + end + + it { should eq ['a_spec.rb', 'b_spec.rb'] } + end + end + end + end + + context 'when not successful 2nd request to API' do + let(:response2_success?) { false } + let(:response2_errors?) { false } + let(:response2) { nil } + + it_behaves_like 'when connection to API failed (fallback mode)' + end + end + end + end + + context 'when not successful request to API' do + let(:success?) { false } + let(:errors?) { false } + + it_behaves_like 'when connection to API failed (fallback mode)' + end + end + + context 'when can_initialize_queue=false' do + let(:can_initialize_queue) { false } + let(:api_code) { nil } + + before do + encrypted_branch = double + expect(KnapsackPro::Crypto::BranchEncryptor).to receive(:call).with(repository_adapter.branch).and_return(encrypted_branch) + + action = double + expect(KnapsackPro::Client::API::V1::Queues).to receive(:queue).with( + can_initialize_queue: can_initialize_queue, + attempt_connect_to_queue: false, # when can_initialize_queue=false then expect attempt_connect_to_queue=false + commit_hash: repository_adapter.commit_hash, + branch: encrypted_branch, + node_total: ci_node_total, + node_index: ci_node_index, + node_build_id: ci_node_build_id, + test_files: nil, # when can_initialize_queue=false then expect test_files is nil to make fast request to API + ).and_return(action) + + connection = instance_double(KnapsackPro::Client::Connection, + call: response, + success?: success?, + errors?: errors?, + api_code: api_code) + expect(KnapsackPro::Client::Connection).to receive(:new).with(action).and_return(connection) + end + + context 'when successful request to API' do + let(:success?) { true } + + context 'when response has errors' do + let(:errors?) { true } + + it do + expect { subject }.to raise_error(ArgumentError) + end + end + + context 'when response has no errors' do + let(:errors?) { false } + + context 'when response returns test files (successful attempt to connect to queue already existing on the API side)' do + let(:test_files) do + [ + { 'path' => 'a_spec.rb' }, + { 'path' => 'b_spec.rb' }, + ] + end + let(:response) do + { 'test_files' => test_files } + end + + context 'when test files encryption is enabled' do + before do + expect(KnapsackPro::Config::Env).to receive(:test_files_encrypted?).and_return(true) + expect(KnapsackPro::Crypto::Decryptor).to receive(:call).with(lazy_loaded_fast_and_slow_test_files_to_run, response['test_files']).and_return(test_files) + end + + it { should eq ['a_spec.rb', 'b_spec.rb'] } + end + + context 'when test files encryption is disabled' do + before do + expect(KnapsackPro::Config::Env).to receive(:test_files_encrypted?).and_return(false) + end + + it { should eq ['a_spec.rb', 'b_spec.rb'] } + end + end + end + end + + context 'when not successful request to API' do + let(:success?) { false } + let(:errors?) { false } + + it_behaves_like 'when connection to API failed (fallback mode)' + end + end end end