(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.

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

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)

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

spec/languageserver/integration/puppet-languageserver/global_queues/sidecar_queue_spec.rb → 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' }
 

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