From 15699678d73f07948611cc0a1d5b61ef00a560c1 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Fri, 6 Dec 2019 15:13:25 +0800 Subject: [PATCH] (GH-209) Refactor single instance queue tests Now that the sidecar and validation queue inherit from the SingleInstanceQueue (SIQ), the common tests can be removed from those queues and applied only the SIQ. This commit only leaves the queue specific details in the queue specific test files, for example the validation queue tests only assert on validation based tasks, not that the queue is single instance. This commit also updates the SingleInstanceQueueJob so that they key is passed in at creation time, instead of requiring overriding an abstract method. The key is immutable so there's no need to make it changeable. --- .../global_queues/sidecar_queue.rb | 5 +- .../global_queues/single_instance_queue.rb | 12 +- .../global_queues/validation_queue.rb | 6 +- .../global_queues/sidecar_queue_spec.rb | 91 +++----- .../single_instance_queue_spec.rb | 204 ++++++++++++++++++ .../global_queues/validation_queue_spec.rb | 27 ++- 6 files changed, 270 insertions(+), 75 deletions(-) rename spec/languageserver/{integration => unit}/puppet-languageserver/global_queues/sidecar_queue_spec.rb (52%) create mode 100644 spec/languageserver/unit/puppet-languageserver/global_queues/single_instance_queue_spec.rb diff --git a/lib/puppet-languageserver/global_queues/sidecar_queue.rb b/lib/puppet-languageserver/global_queues/sidecar_queue.rb index 610086c9..ba5c07d4 100644 --- a/lib/puppet-languageserver/global_queues/sidecar_queue.rb +++ b/lib/puppet-languageserver/global_queues/sidecar_queue.rb @@ -13,15 +13,12 @@ class SidecarQueueJob < SingleInstanceQueueJob attr_accessor :connection_id def initialize(action, additional_args, handle_errors, connection_id) + super("#{action}-#{connection_id}") @action = action @additional_args = additional_args @handle_errors = handle_errors @connection_id = connection_id end - - def key - "#{action}-#{connection_id}" - end end # Module for enqueing and running sidecar jobs asynchronously diff --git a/lib/puppet-languageserver/global_queues/single_instance_queue.rb b/lib/puppet-languageserver/global_queues/single_instance_queue.rb index 478322fc..49d9fcb0 100644 --- a/lib/puppet-languageserver/global_queues/single_instance_queue.rb +++ b/lib/puppet-languageserver/global_queues/single_instance_queue.rb @@ -3,13 +3,12 @@ module PuppetLanguageServer module GlobalQueues class SingleInstanceQueueJob - def initialize(*_); end - # Unique key for the job. The SingleInstanceQueue uses the key # to ensure that only a single instance is in the queue - # @api asbtract - def key - raise ArgumentError, "key should be implenmented for #{self.class}" + attr_reader :key + + def initialize(key) + @key = key end end @@ -30,9 +29,10 @@ def max_queue_threads end # The ruby Job class that this queue operates on + # Should be inherited from SingleInstanceQueueJob # @api asbtract def job_class - raise ArgumentError, "job_class should be implemented for #{self.class}" + SingleInstanceQueueJob end def new_job(*args) diff --git a/lib/puppet-languageserver/global_queues/validation_queue.rb b/lib/puppet-languageserver/global_queues/validation_queue.rb index dafeae3b..a7ffe0ae 100644 --- a/lib/puppet-languageserver/global_queues/validation_queue.rb +++ b/lib/puppet-languageserver/global_queues/validation_queue.rb @@ -12,16 +12,12 @@ class ValidationQueueJob < SingleInstanceQueueJob attr_accessor :options def initialize(file_uri, doc_version, connection_id, options = {}) - super + super(file_uri) @file_uri = file_uri @doc_version = doc_version @connection_id = connection_id @options = options end - - def key - @file_uri - end end # Class for enqueing and running document level validation asynchronously diff --git a/spec/languageserver/integration/puppet-languageserver/global_queues/sidecar_queue_spec.rb b/spec/languageserver/unit/puppet-languageserver/global_queues/sidecar_queue_spec.rb similarity index 52% rename from spec/languageserver/integration/puppet-languageserver/global_queues/sidecar_queue_spec.rb rename to spec/languageserver/unit/puppet-languageserver/global_queues/sidecar_queue_spec.rb index 532161aa..2905fd3d 100644 --- a/spec/languageserver/integration/puppet-languageserver/global_queues/sidecar_queue_spec.rb +++ b/spec/languageserver/unit/puppet-languageserver/global_queues/sidecar_queue_spec.rb @@ -1,80 +1,53 @@ require 'spec_helper' +require 'puppet-languageserver/session_state/document_store' -class SuccessStatus - def exitstatus - 0 +describe 'PuppetLanguageServer::GlobalQueues::SidecarQueueJob' do + let(:action) { 'action' } + let(:additional_args) { [] } + let(:handle_errors) { false } + let(:connection_id) { 'id1234' } + + let(:subject) { PuppetLanguageServer::GlobalQueues::SidecarQueueJob.new(action, additional_args, handle_errors, connection_id) } + + it 'is a SingleInstanceQueueJob' do + expect(subject).is_a?(PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob) + end + + it 'uses the action and connection_id for the key' do + expect(subject.key).to eq("#{action}-#{connection_id}") end end -describe 'sidecar_queue' do - let(:mock_connection) { Object.new } - let(:connection_id) { 'mock_conn_id' } - let(:cache) { PuppetLanguageServer::SessionState::ObjectCache.new } - let(:session_state) { PuppetLanguageServer::ClientSessionState.new(nil, :object_cache => cache, :connection_id => connection_id) } +describe 'PuppetLanguageServer::GlobalQueues::SidecarQueue' do let(:subject) { PuppetLanguageServer::GlobalQueues::SidecarQueue.new } - before(:each) do - # Mock a connection and session state - allow(subject).to receive(:connection_from_connection_id).with(connection_id).and_return(mock_connection) - allow(subject).to receive(:sidecar_args_from_connection).with(mock_connection).and_return([]) - allow(subject).to receive(:session_state_from_connection).with(mock_connection).and_return(session_state) + it 'is a SingleInstanceQueue' do + expect(subject).is_a?(PuppetLanguageServer::GlobalQueues::SingleInstanceQueue) end - describe 'SidecarQueueJob' do - it 'should only contain the action name connection id in the key' do - job = PuppetLanguageServer::GlobalQueues::SidecarQueueJob.new('action', ['additional'], true, connection_id) - expect(job.key).to eq("action-#{connection_id}") - end + it 'has a job_class of SidecarQueueJob' do + expect(subject.job_class).is_a?(PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob) end - describe '#enqueue' do - let(:action1) { 'default_classes' } - let(:action2) { 'default_types' } - let(:additional_args1) { [] } - let(:additional_args2) { [] } + describe '#execute' do + let(:mock_connection) { Object.new } + let(:connection_id) { 'mock_conn_id' } + let(:cache) { PuppetLanguageServer::SessionState::ObjectCache.new } + let(:session_state) { PuppetLanguageServer::ClientSessionState.new(nil, :object_cache => cache, :connection_id => connection_id) } before(:each) do - allow(subject).to receive(:execute_job).and_raise("#{self.class}.execute_job mock should not be called") + # Mock a connection and session state + allow(subject).to receive(:connection_from_connection_id).with(connection_id).and_return(mock_connection) + allow(subject).to receive(:sidecar_args_from_connection).with(mock_connection).and_return([]) + allow(subject).to receive(:session_state_from_connection).with(mock_connection).and_return(session_state) end - context 'for a single item in the queue' do - it 'should call the synchronous method' do - expect(subject).to receive(:execute_job).with(having_attributes( - :action => action1, - :additional_args => additional_args1, - :handle_errors => false, - :connection_id => connection_id - )).and_return(true) - - subject.enqueue(action1, additional_args1, false, connection_id) - subject.drain_queue + class SuccessStatus + def exitstatus + 0 end end - context 'for multiple items in the queue' do - it 'should process all unique actions' do - expect(subject).to receive(:execute_job).with(having_attributes( - :action => action1, - :additional_args => additional_args1, - :handle_errors => false, - :connection_id => connection_id - )).and_return(true) - - expect(subject).to receive(:execute_job).with(having_attributes( - :action => action2, - :additional_args => additional_args2, - :handle_errors => false, - :connection_id => connection_id - )).and_return(true) - - subject.enqueue(action1, additional_args1, false, connection_id) - subject.enqueue(action2, additional_args2, false, connection_id) - subject.drain_queue - end - end - end - - describe '#execute' do context 'default_aggregate action' do let(:action) { 'default_aggregate' } diff --git a/spec/languageserver/unit/puppet-languageserver/global_queues/single_instance_queue_spec.rb b/spec/languageserver/unit/puppet-languageserver/global_queues/single_instance_queue_spec.rb new file mode 100644 index 00000000..bba5f2c0 --- /dev/null +++ b/spec/languageserver/unit/puppet-languageserver/global_queues/single_instance_queue_spec.rb @@ -0,0 +1,204 @@ +require 'spec_helper' + +# A queue that will pause job execution if the pause_queue is set to true +class PausableQueue < PuppetLanguageServer::GlobalQueues::SingleInstanceQueue + attr_accessor :pause_queue + + def initialize(max_queue_threads) + super() + @max_queue_threads = max_queue_threads + @pause_queue = false + end + + def max_queue_threads + @max_queue_threads + end + + def execute_job(_) + super + begin + sleep 1 if pause_queue + end while pause_queue + end +end + +describe 'PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob' do + let(:key) { 'a key'} + let(:subject) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new(key) } + + it 'has a method called key' do + expect(subject).to respond_to(:key) + end + + it 'returns the key' do + expect(subject.key).to eq(key) + end +end + +describe 'PuppetLanguageServer::GlobalQueues::SingleInstanceQueue' do + let(:subject) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueue.new } + + describe '#max_queue_threads' do + let(:job1) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job1') } + let(:job2) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job2') } + let(:job3) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job3') } + let(:job4) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job4') } + + it 'has a method called max_queue_threads' do + expect(subject).to respond_to(:max_queue_threads) + end + + it 'executes the specified number of threads' do + queue = PausableQueue.new(2) + + # Enqueue four jobs + queue.pause_queue = true + queue.enqueue_job(job1) + queue.enqueue_job(job2) + queue.enqueue_job(job3) + queue.enqueue_job(job4) + + # Give the queue a few moments to start up + sleep(2) + + # Because we have 2 job threads, the first two jobs will no longer be in the + # queue, and jobs 3 and 4 should be + # Note - using instance_variable_get isn't the greatest but it works + internal_queue = queue.instance_variable_get(:@queue) + expect(internal_queue.count).to eq(2) + expect(internal_queue[0]).to be(job3) + expect(internal_queue[1]).to be(job4) + + queue.pause_queue = false + queue.drain_queue + end + end + + describe '#job_class' do + it 'defaults to SingleInstanceQueueJob' do + expect(subject.job_class).to eq(PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob) + end + end + + describe '#newjob' do + it 'should create a new job object' do + expect(subject.new_job('new_job_key')).is_a?(PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob) + expect(subject.new_job('new_job_key').key).to eq('new_job_key') + end + end + + describe '#enqueue' do + it 'should call enqueue_job with the new job object' do + expect(subject).to receive(:enqueue_job).with(having_attributes(:key => 'test_enqueue')) + subject.enqueue('test_enqueue') + subject.drain_queue + end + end + + describe '#enqueue_job' do + it 'raises if the job object is the wrong type' do + expect{ subject.enqueue_job(Object.new) }.to raise_error(RuntimeError) + end + + context 'with an empty queue' do + let(:job) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job') } + it 'should execute a new job only once' do + expect(subject).to receive(:execute_job).with(job).once + + subject.enqueue_job(job) + subject.drain_queue + end + end + + context 'with mulitple item in the queue' do + let(:job1) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job1') } + let(:job2) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job2') } + let(:job3) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job3') } + let(:job4) { PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob.new('single_job4') } + + before(:each) do + subject.reset_queue([job1, job2, job3]) + end + + it 'enqueues jobs even if they\'re already procssing' do + queue = PausableQueue.new(1) + + # Enqueue three jobs + queue.pause_queue = true + queue.enqueue_job(job1) + queue.enqueue_job(job2) + queue.enqueue_job(job3) + + # Give the queue a few moments to start up + sleep(2) + + # Because we have 1 job threads, the first jos will no longer be in the + # queue, and jobs 2, 3 and 4 should be + # Note - using instance_variable_get isn't the greatest but it works + internal_queue = queue.instance_variable_get(:@queue) + expect(internal_queue.count).to eq(2) + + # Enqueue Job 1 again + queue.enqueue_job(job1) + + # Job 1 should now be at the end of queue + internal_queue = queue.instance_variable_get(:@queue) + expect(internal_queue.count).to eq(3) + expect(internal_queue[2]).to be(job1) + + queue.pause_queue = false + queue.drain_queue + end + + it 'should execute new jobs only once' do + expect(subject).to receive(:execute_job).with(job1).once + expect(subject).to receive(:execute_job).with(job2).once + expect(subject).to receive(:execute_job).with(job3).once + expect(subject).to receive(:execute_job).with(job4).once + + subject.enqueue_job(job4) + subject.drain_queue + end + + it 'should not execute jobs already in the queue' do + expect(subject).to receive(:execute_job).with(job1).once + expect(subject).to receive(:execute_job).with(job2).once + expect(subject).to receive(:execute_job).with(job3).once + + subject.enqueue_job(job3) + subject.drain_queue + end + + it 'should execute all jobs even if they raise errors' do + expect(subject).to receive(:execute_job).with(job1).once.and_raise('Job1 Error') + expect(subject).to receive(:execute_job).with(job2).once.and_raise('Job2 Error') + expect(subject).to receive(:execute_job).with(job3).once.and_raise('Job3 Error') + + subject.enqueue_job(job3) + subject.drain_queue + end + end + + end + + describe '#execute' do + it 'should call execute_job with the new job object' do + expect(subject).to receive(:execute_job).with(having_attributes(:key => 'test_enqueue')) + subject.execute('test_enqueue') + end + end + + describe '#execute_job' do + it 'raises if the job object is the wrong type' do + expect{ subject.execute_job(Object.new) }.to raise_error(RuntimeError) + end + end + + describe '#drain_queue' do + # Does not need to be tested directly. It's used in other parts of this unit test file + end + + describe '#reset_queue' do + # Does not need to be tested directly. It's used in other parts of this unit test file + end +end diff --git a/spec/languageserver/unit/puppet-languageserver/global_queues/validation_queue_spec.rb b/spec/languageserver/unit/puppet-languageserver/global_queues/validation_queue_spec.rb index ed011145..558af8d5 100644 --- a/spec/languageserver/unit/puppet-languageserver/global_queues/validation_queue_spec.rb +++ b/spec/languageserver/unit/puppet-languageserver/global_queues/validation_queue_spec.rb @@ -1,7 +1,24 @@ require 'spec_helper' require 'puppet-languageserver/session_state/document_store' -describe 'validation_queue' do +describe 'PuppetLanguageServer::GlobalQueues::ValidationQueueJob' do + let(:file_uri) { 'file_uri' } + let(:doc_version) { 1 } + let(:connection_id) { 'id1234' } + let(:options) { {} } + + let(:subject) { PuppetLanguageServer::GlobalQueues::ValidationQueueJob.new(file_uri, doc_version, connection_id, options) } + + it 'is a SingleInstanceQueueJob' do + expect(subject).is_a?(PuppetLanguageServer::GlobalQueues::SingleInstanceQueueJob) + end + + it 'uses the file uri as the job key' do + expect(subject.key).to eq(file_uri) + end +end + +describe 'PuppetLanguageServer::GlobalQueues::ValidationQueue' do VALIDATE_MANIFEST_FILENAME = 'file:///something.pp' VALIDATE_PUPPETFILE_FILENAME = 'file:///Puppetfile' VALIDATE_EPP_FILENAME = 'file:///something.epp' @@ -24,6 +41,14 @@ def job(file_uri, document_version, connection_id, job_options = {}) PuppetLanguageServer::GlobalQueues::ValidationQueueJob.new(file_uri, document_version, connection_id, job_options) end + it 'is a SingleInstanceQueue' do + expect(subject).is_a?(PuppetLanguageServer::GlobalQueues::SingleInstanceQueue) + end + + it 'has a job_class of ValidationQueueJob' do + expect(subject.job_class).is_a?(PuppetLanguageServer::GlobalQueues::ValidationQueueJob) + end + describe '#enqueue' do shared_examples_for "single document which sends validation results" do |file_uri, file_content, validation_result| it 'should send validation results' do