From 8bcf74872a9cb2d1144ef2d2d80d38a23d6e5783 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 13:43:26 -0800
Subject: [PATCH 01/35] (POOLER-73) Restructure tests to unit and integration
 directories

Previously all of the spec tests for VM Pooler were all together in the specs
directory.  However some tests require a working local Redis server to operate
and other instead mock all external dependencies.  This commit splits the test
files between unit and integration, where integration tests require a working
Redis instance, and unit tests do not.  This commit also removes the root
`vmpooler` directory as it is not required.  The tests rake test still operates
correctly.

This commit also adds the mock_redis library for testing for the pool_manager.
---
 spec/{vmpooler => integration}/api/v1/status_spec.rb      | 0
 spec/{vmpooler => integration}/api/v1/token_spec.rb       | 0
 spec/{vmpooler => integration}/api/v1/vm_hostname_spec.rb | 0
 spec/{vmpooler => integration}/api/v1/vm_spec.rb          | 0
 spec/{vmpooler => integration}/api/v1/vm_template_spec.rb | 0
 spec/{vmpooler => integration}/dashboard_spec.rb          | 0
 spec/{vmpooler => unit}/api/helpers_spec.rb               | 0
 spec/{vmpooler => unit}/pool_manager_migration_spec.rb    | 0
 spec/{vmpooler => unit}/pool_manager_spec.rb              | 1 +
 9 files changed, 1 insertion(+)
 rename spec/{vmpooler => integration}/api/v1/status_spec.rb (100%)
 rename spec/{vmpooler => integration}/api/v1/token_spec.rb (100%)
 rename spec/{vmpooler => integration}/api/v1/vm_hostname_spec.rb (100%)
 rename spec/{vmpooler => integration}/api/v1/vm_spec.rb (100%)
 rename spec/{vmpooler => integration}/api/v1/vm_template_spec.rb (100%)
 rename spec/{vmpooler => integration}/dashboard_spec.rb (100%)
 rename spec/{vmpooler => unit}/api/helpers_spec.rb (100%)
 rename spec/{vmpooler => unit}/pool_manager_migration_spec.rb (100%)
 rename spec/{vmpooler => unit}/pool_manager_spec.rb (99%)

diff --git a/spec/vmpooler/api/v1/status_spec.rb b/spec/integration/api/v1/status_spec.rb
similarity index 100%
rename from spec/vmpooler/api/v1/status_spec.rb
rename to spec/integration/api/v1/status_spec.rb
diff --git a/spec/vmpooler/api/v1/token_spec.rb b/spec/integration/api/v1/token_spec.rb
similarity index 100%
rename from spec/vmpooler/api/v1/token_spec.rb
rename to spec/integration/api/v1/token_spec.rb
diff --git a/spec/vmpooler/api/v1/vm_hostname_spec.rb b/spec/integration/api/v1/vm_hostname_spec.rb
similarity index 100%
rename from spec/vmpooler/api/v1/vm_hostname_spec.rb
rename to spec/integration/api/v1/vm_hostname_spec.rb
diff --git a/spec/vmpooler/api/v1/vm_spec.rb b/spec/integration/api/v1/vm_spec.rb
similarity index 100%
rename from spec/vmpooler/api/v1/vm_spec.rb
rename to spec/integration/api/v1/vm_spec.rb
diff --git a/spec/vmpooler/api/v1/vm_template_spec.rb b/spec/integration/api/v1/vm_template_spec.rb
similarity index 100%
rename from spec/vmpooler/api/v1/vm_template_spec.rb
rename to spec/integration/api/v1/vm_template_spec.rb
diff --git a/spec/vmpooler/dashboard_spec.rb b/spec/integration/dashboard_spec.rb
similarity index 100%
rename from spec/vmpooler/dashboard_spec.rb
rename to spec/integration/dashboard_spec.rb
diff --git a/spec/vmpooler/api/helpers_spec.rb b/spec/unit/api/helpers_spec.rb
similarity index 100%
rename from spec/vmpooler/api/helpers_spec.rb
rename to spec/unit/api/helpers_spec.rb
diff --git a/spec/vmpooler/pool_manager_migration_spec.rb b/spec/unit/pool_manager_migration_spec.rb
similarity index 100%
rename from spec/vmpooler/pool_manager_migration_spec.rb
rename to spec/unit/pool_manager_migration_spec.rb
diff --git a/spec/vmpooler/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
similarity index 99%
rename from spec/vmpooler/pool_manager_spec.rb
rename to spec/unit/pool_manager_spec.rb
index 6bf00bd72..5f16bf3ff 100644
--- a/spec/vmpooler/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1,5 +1,6 @@
 require 'spec_helper'
 require 'time'
+require 'mock_redis'
 
 describe 'Pool Manager' do
   let(:logger) { double('logger') }

From a9fc1bb8aa0416811db4a7348f22e2d76f2aa821 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 11:06:36 -0800
Subject: [PATCH 02/35] (POOLER-73) Update test helpers

This commit adds the following test helpers:
- MockFindFolder
  Returns an mock result object from calling `Vmpooler::VsphereHelper.find_folder(foldername)`
- Use MockRedis instead of using method stubs
- MockLogger
  Creates an object which looks like the VMPooler::Logger object but just
  ignores all messages.  This stops the proliferation of allow(logger).to ....
  expectations in tests
- create_completed_vm
  Creates the required redis information for a completed VM
- create_discovered_vm
  Creates the required redis information for a newly discovered VM
- snapshot_revert_vm
  Creates the required redis information to revert a snapshot for a VM
- disk_task_vm
   Creates the required redis information to add a disk addition task to a VM
---
 spec/helpers.rb                | 43 ++++++++++++++++++++++++++++++++++
 spec/unit/pool_manager_spec.rb |  4 ++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/spec/helpers.rb b/spec/helpers.rb
index 9005ec57e..7fda0d9b3 100644
--- a/spec/helpers.rb
+++ b/spec/helpers.rb
@@ -6,6 +6,28 @@ def redis
   @redis
 end
 
+# Mock an object which is result from Vmpooler::VsphereHelper.find_folder(foldername)
+class MockFindFolder
+  attr_reader :childEntity
+
+  def initialize(vmlist = [])
+    # Generate an array of hashes
+    @childEntity = vmlist.map do |vm|
+      vm_object = {}
+      vm_object['name'] = vm
+
+      vm_object
+    end
+  end
+end
+
+# Mock an object which represents a Logger.  This stops the proliferation
+# of allow(logger).to .... expectations in tests.
+class MockLogger
+  def log(_level, string)
+  end
+end
+
 def expect_json(ok = true, http = 200)
   expect(last_response.header['Content-Type']).to eq('application/json')
 
@@ -56,6 +78,18 @@ def create_vm(name, token = nil, redis_handle = nil)
   redis_db.hset("vmpooler__vm__#{name}", 'token:token', token) if token
 end
 
+def create_completed_vm(name, pool, active = false, redis_handle = nil)
+  redis_db = redis_handle ? redis_handle : redis
+  redis_db.sadd("vmpooler__completed__#{pool}", name)
+  redis_db.hset("vmpooler__vm__#{name}", 'checkout', Time.now)
+  redis_db.hset("vmpooler__active__#{pool}", name, Time.now) if active
+end
+
+def create_discovered_vm(name, pool, redis_handle = nil)
+  redis_db = redis_handle ? redis_handle : redis
+  redis_db.sadd("vmpooler__discovered__#{pool}", name)
+end
+
 def create_migrating_vm(name, pool, redis_handle = nil)
   redis_db = redis_handle ? redis_handle : redis
   redis_db.hset("vmpooler__vm__#{name}", 'checkout', Time.now)
@@ -71,11 +105,20 @@ def fetch_vm(vm)
   redis.hgetall("vmpooler__vm__#{vm}")
 end
 
+def snapshot_revert_vm(vm, snapshot = '12345678901234567890123456789012')
+  redis.sadd('vmpooler__tasks__snapshot-revert', "#{vm}:#{snapshot}")
+  redis.hset("vmpooler__vm__#{vm}", "snapshot:#{snapshot}", "1")
+end
+
 def snapshot_vm(vm, snapshot = '12345678901234567890123456789012')
   redis.sadd('vmpooler__tasks__snapshot', "#{vm}:#{snapshot}")
   redis.hset("vmpooler__vm__#{vm}", "snapshot:#{snapshot}", "1")
 end
 
+def disk_task_vm(vm, disk_size = '10')
+  redis.sadd('vmpooler__tasks__disk', "#{vm}:#{disk_size}")
+end
+
 def has_vm_snapshot?(vm)
   redis.smembers('vmpooler__tasks__snapshot').any? do |snapshot|
     instance, sha = snapshot.split(':')
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 5f16bf3ff..de42fa6f5 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -3,8 +3,8 @@
 require 'mock_redis'
 
 describe 'Pool Manager' do
-  let(:logger) { double('logger') }
-  let(:redis) { double('redis') }
+  let(:logger) { MockLogger.new }
+  let(:redis) { MockRedis.new }
   let(:metrics) { Vmpooler::DummyStatsd.new }
   let(:config) { {} }
   let(:pool) { 'pool1' }

From af9ec66b787b31b2cd7e4e7931dea1cbad89d946 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 16:54:52 -0800
Subject: [PATCH 03/35] (POOLER-73) Add spec tests for check_pending_vm

Add spec tests for check_pending_vm
---
 spec/unit/pool_manager_spec.rb | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index de42fa6f5..65cfa784b 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -14,6 +14,21 @@
 
   subject { Vmpooler::PoolManager.new(config, logger, redis, metrics) }
 
+  describe '#check_pending_vm' do
+    let(:vsphere) { double('vsphere') }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'calls _check_pending_vm' do
+      expect(Thread).to receive(:new).and_yield
+      expect(subject).to receive(:_check_pending_vm).with(vm,pool,timeout,vsphere)
+
+      subject.check_pending_vm(vm, pool, timeout, vsphere)
+    end
+  end
+
   describe '#_check_pending_vm' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From b72275b5525ead788e4f73ef381eee12b6431f4f Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 16:56:00 -0800
Subject: [PATCH 04/35] (POOLER-73) Add spec tests for open_socket

 Add spec tests for open_socket
---
 spec/unit/pool_manager_spec.rb | 56 ++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 65cfa784b..239cddf2a 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -29,6 +29,62 @@
     end
   end
 
+  describe '#open_socket' do
+    let(:TCPSocket) { double('tcpsocket') }
+    let(:socket) { double('tcpsocket') }
+    let(:hostname) { 'host' }
+    let(:domain) { 'domain.local'}
+    let(:default_socket) { 22 }
+
+    before do
+      expect(subject).not_to be_nil
+      allow(socket).to receive(:close)
+    end
+
+    it 'opens socket with defaults' do
+      expect(TCPSocket).to receive(:new).with(hostname,default_socket).and_return(socket)
+
+      expect(subject.open_socket(hostname)).to eq(nil)
+    end
+
+    it 'yields the socket if a block is given' do
+      expect(TCPSocket).to receive(:new).with(hostname,default_socket).and_return(socket)
+
+      expect{ |socket| subject.open_socket(hostname,nil,nil,default_socket,&socket) }.to yield_control.exactly(1).times 
+    end
+
+    it 'closes the opened socket' do
+      expect(TCPSocket).to receive(:new).with(hostname,default_socket).and_return(socket)
+      expect(socket).to receive(:close)
+
+      expect(subject.open_socket(hostname)).to eq(nil)
+    end
+
+    it 'opens a specific socket' do
+      expect(TCPSocket).to receive(:new).with(hostname,80).and_return(socket)
+
+      expect(subject.open_socket(hostname,nil,nil,80)).to eq(nil)
+    end
+
+    it 'uses a specific domain with the hostname' do
+      expect(TCPSocket).to receive(:new).with("#{hostname}.#{domain}",default_socket).and_return(socket)
+
+      expect(subject.open_socket(hostname,domain)).to eq(nil)
+    end
+
+    it 'raises error if host is not resolvable' do
+      expect(TCPSocket).to receive(:new).with(hostname,default_socket).and_raise(SocketError,'getaddrinfo: No such host is known')
+
+      expect { subject.open_socket(hostname,nil,1) }.to raise_error(SocketError)
+    end
+
+    it 'raises error if socket is not listening' do
+      expect(TCPSocket).to receive(:new).with(hostname,default_socket).and_raise(SocketError,'No connection could be made because the target machine actively refused it')
+
+      expect { subject.open_socket(hostname,nil,1) }.to raise_error(SocketError)
+    end
+  end
+
   describe '#_check_pending_vm' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From 9fdb51eb00c8bafcd32f89b45d71b2bb7533a921 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:00:12 -0800
Subject: [PATCH 05/35] (POOLER-73) Modify spec tests for _check_pending_vm

(POOLER-73) Modify spec tests for _check_pending_vm
---
 spec/unit/pool_manager_spec.rb | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 239cddf2a..595754451 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -86,33 +86,34 @@
   end
 
   describe '#_check_pending_vm' do
-    let(:pool_helper) { double('pool') }
-    let(:vsphere) { {pool => pool_helper} }
+    let(:vsphere) { double('vsphere') }
 
     before do
       expect(subject).not_to be_nil
-      $vsphere = vsphere
     end
 
-    context 'host not in pool' do
+    context 'host does not exist or not in pool' do
       it 'calls fail_pending_vm' do
-        allow(vsphere).to receive(:find_vm).and_return(nil)
-        allow(redis).to receive(:hget)
+        expect(vsphere).to receive(:find_vm).and_return(nil)
+        expect(subject).to receive(:fail_pending_vm).with(vm, pool, timeout, false) 
+
         subject._check_pending_vm(vm, pool, timeout, vsphere)
       end
     end
 
     context 'host is in pool' do
-      let(:vm_finder) { double('vm_finder') }
-      let(:tcpsocket) { double('TCPSocket') }
+      it 'calls move_pending_vm_to_ready if host is ready' do
+        expect(vsphere).to receive(:find_vm).and_return(host)
+        expect(subject).to receive(:open_socket).and_return(nil)
+        expect(subject).to receive(:move_pending_vm_to_ready).with(vm, pool, host)
 
-      it 'calls move_pending_vm_to_ready' do
-        allow(subject).to receive(:open_socket).and_return(true)
-        allow(vsphere).to receive(:find_vm).and_return(vm_finder)
-        allow(vm_finder).to receive(:summary).and_return(nil)
+        subject._check_pending_vm(vm, pool, timeout, vsphere)
+      end
 
-        expect(vm_finder).to receive(:summary).once
-        expect(redis).not_to receive(:hget).with(String, 'clone')
+      it 'calls fail_pending_vm if an error is raised' do
+        expect(vsphere).to receive(:find_vm).and_return(host)
+        expect(subject).to receive(:open_socket).and_raise(SocketError,'getaddrinfo: No such host is known')
+        expect(subject).to receive(:fail_pending_vm).with(vm, pool, timeout)
 
         subject._check_pending_vm(vm, pool, timeout, vsphere)
       end

From b5ec74bae93d7cbaad872d7465b599f0559e9789 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:00:58 -0800
Subject: [PATCH 06/35] (POOLER-73) Add spec tests for remove_nonexistent_vm

Add spec tests for remove_nonexistent_vm
---
 spec/unit/pool_manager_spec.rb | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 595754451..de6e3d2ba 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -120,6 +120,26 @@
     end
   end
 
+  describe '#remove_nonexistent_vm' do
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'removes VM from pending in redis' do
+      create_pending_vm(pool,vm)
+
+      expect(redis.sismember("vmpooler__pending__#{pool}", vm)).to be(true)
+      subject.remove_nonexistent_vm(vm, pool)
+      expect(redis.sismember("vmpooler__pending__#{pool}", vm)).to be(false)
+    end
+
+    it 'logs msg' do
+      expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' no longer exists. Removing from pending.")
+
+      subject.remove_nonexistent_vm(vm, pool)
+    end
+  end
+
   describe '#move_vm_to_ready' do
     before do
       expect(subject).not_to be_nil

From 4b2ca49e02dc793c6266929b1a71a8317b2db68d Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:01:32 -0800
Subject: [PATCH 07/35] (POOLER-73) Modify spec tests for fail_pending_vm

Modify spec tests for fail_pending_vm
---
 spec/unit/pool_manager_spec.rb | 84 +++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 32 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index de6e3d2ba..b48ac65b4 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -140,6 +140,58 @@
     end
   end
 
+ describe '#fail_pending_vm' do
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      create_pending_vm(pool,vm)
+    end
+
+    it 'takes no action if VM is not cloning' do
+      expect(subject.fail_pending_vm(vm, pool, timeout)).to eq(nil)
+      expect(redis.sismember("vmpooler__pending__#{pool}", vm)).to be(true)
+    end
+
+    it 'takes no action if VM is within timeout' do
+      redis.hset("vmpooler__vm__#{vm}", 'clone',Time.now.to_s)
+      expect(subject.fail_pending_vm(vm, pool, timeout)).to eq(nil)
+      expect(redis.sismember("vmpooler__pending__#{pool}", vm)).to be(true)
+    end
+
+    it 'moves VM to completed queue if VM has exceeded timeout and exists' do
+      redis.hset("vmpooler__vm__#{vm}", 'clone',Date.new(2001,1,1).to_s)
+      expect(subject.fail_pending_vm(vm, pool, timeout,true)).to eq(nil)
+      expect(redis.sismember("vmpooler__pending__#{pool}", vm)).to be(false)
+      expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(true)
+    end
+
+    it 'logs message if VM has exceeded timeout and exists' do
+      redis.hset("vmpooler__vm__#{vm}", 'clone',Date.new(2001,1,1).to_s)
+      expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' marked as 'failed' after #{timeout} minutes")
+      expect(subject.fail_pending_vm(vm, pool, timeout,true)).to eq(nil)
+    end
+
+    it 'calls remove_nonexistent_vm if VM has exceeded timeout and does not exist' do
+      redis.hset("vmpooler__vm__#{vm}", 'clone',Date.new(2001,1,1).to_s)
+      expect(subject).to receive(:remove_nonexistent_vm).with(vm, pool)
+      expect(subject.fail_pending_vm(vm, pool, timeout,false)).to eq(nil)
+    end
+
+    it 'swallows error if an error is raised' do
+      redis.hset("vmpooler__vm__#{vm}", 'clone','iamnotparsable_asdate')
+      expect(subject.fail_pending_vm(vm, pool, timeout,true)).to eq(nil)
+    end
+
+    it 'logs message if an error is raised' do
+      redis.hset("vmpooler__vm__#{vm}", 'clone','iamnotparsable_asdate')
+      expect(logger).to receive(:log).with('d', String)
+
+      subject.fail_pending_vm(vm, pool, timeout,true)
+    end
+  end
+  
   describe '#move_vm_to_ready' do
     before do
       expect(subject).not_to be_nil
@@ -204,38 +256,6 @@
     end
   end
 
-  describe '#fail_pending_vm' do
-    before do
-      expect(subject).not_to be_nil
-    end
-
-    context 'does not have a clone stamp' do
-      it 'has no side effects' do
-        allow(redis).to receive(:hget)
-        subject.fail_pending_vm(vm, pool, timeout)
-      end
-    end
-
-    context 'has valid clone stamp' do
-      it 'does nothing when less than timeout' do
-        allow(redis).to receive(:hget).with(String, 'clone').and_return Time.now.to_s
-        subject.fail_pending_vm(vm, pool, timeout)
-      end
-
-      it 'moves vm to completed when over timeout' do
-        allow(redis).to receive(:hget).with(String, 'clone').and_return '2005-01-1'
-        allow(redis).to receive(:smove).with(String, String, String)
-        allow(logger).to receive(:log).with(String, String)
-
-        expect(redis).to receive(:smove).with(String, String, vm)
-        expect(logger).to receive(:log).with('d', String)
-
-        subject.fail_pending_vm(vm, pool, timeout)
-      end
-
-    end
-  end
-
   describe '#_check_running_vm' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From 8ae6b29be3450bd67d732392c5af66b8fd1fdf45 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:06:41 -0800
Subject: [PATCH 08/35] (POOLER-73) Modify spec tests for
 move_pending_vm_to_ready

Modify spec tests for move_pending_vm_to_ready
---
 spec/unit/pool_manager_spec.rb | 79 +++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 39 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index b48ac65b4..366579235 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -192,66 +192,67 @@
     end
   end
   
-  describe '#move_vm_to_ready' do
+  describe '#move_pending_vm_to_ready' do
     before do
       expect(subject).not_to be_nil
+      allow(Socket).to receive(:getaddrinfo)
     end
 
-    context 'a host without correct summary' do
-      it 'does nothing when summary is nil' do
-        allow(host).to receive(:summary).and_return nil
-        subject.move_pending_vm_to_ready(vm, pool, host)
-      end
+    before(:each) do
+      create_pending_vm(pool,vm)
+    end
 
-      it 'does nothing when guest is nil' do
-        allow(host).to receive(:summary).and_return true
-        allow(host).to receive_message_chain(:summary, :guest).and_return nil
-        subject.move_pending_vm_to_ready(vm, pool, host)
-      end
+    context 'when hostname does not match VM name' do
+      it 'should not take any action' do
+        expect(logger).to receive(:log).exactly(0).times
+        expect(Socket).to receive(:getaddrinfo).exactly(0).times
 
-      it 'does nothing when hostName is nil' do
-        allow(host).to receive(:summary).and_return true
-        allow(host).to receive_message_chain(:summary, :guest).and_return true
-        allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return nil
-        subject.move_pending_vm_to_ready(vm, pool, host)
-      end
+        allow(host).to receive(:summary).and_return( double('summary') )
+        allow(host).to receive_message_chain(:summary, :guest).and_return( double('guest') )
+        allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return ('different_name')
 
-      it 'does nothing when hostName does not match vm' do
-        allow(host).to receive(:summary).and_return true
-        allow(host).to receive_message_chain(:summary, :guest).and_return true
-        allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return 'adifferentvm'
         subject.move_pending_vm_to_ready(vm, pool, host)
       end
     end
 
-    context 'a host with proper summary' do
+    context 'when hostname matches VM name' do
       before do
-        allow(host).to receive(:summary).and_return true
-        allow(host).to receive_message_chain(:summary, :guest).and_return true
-        allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return vm
-
-        allow(redis).to receive(:hget)
-        allow(redis).to receive(:smove)
-        allow(redis).to receive(:hset)
-        allow(logger).to receive(:log)
+        allow(host).to receive(:summary).and_return( double('summary') )
+        allow(host).to receive_message_chain(:summary, :guest).and_return( double('guest') )
+        allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return (vm)
       end
 
-      it 'moves vm to ready' do
-        allow(redis).to receive(:hget).with(String, 'clone').and_return Time.now.to_s
+      it 'should move the VM from pending to ready pool' do
+        expect(redis.sismember("vmpooler__pending__#{pool}", vm)).to be(true)
+        expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(false)
+        subject.move_pending_vm_to_ready(vm, pool, host)
+        expect(redis.sismember("vmpooler__pending__#{pool}", vm)).to be(false)
+        expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(true)
+      end
 
-        expect(redis).to receive(:smove).with(String, String, vm)
-        expect(redis).to receive(:hset).with(String, String, String)
-        expect(logger).to receive(:log).with('s', String)
+      it 'should log a message' do
+        expect(logger).to receive(:log).with('s', "[>] [#{pool}] '#{vm}' moved to 'ready' queue")
 
         subject.move_pending_vm_to_ready(vm, pool, host)
       end
 
-      it 'sets finish to nil when clone_time is nil' do
-        expect(redis).to receive(:smove).with(String, String, vm)
-        expect(redis).to receive(:hset).with(String, String, nil)
-        expect(logger).to receive(:log).with('s', String)
+      it 'should set the boot time in redis' do
+        redis.hset("vmpooler__vm__#{vm}", 'clone',Time.now.to_s)
+        expect(redis.hget('vmpooler__boot__' + Date.today.to_s, pool + ':' + vm)).to be_nil
+        subject.move_pending_vm_to_ready(vm, pool, host)
+        expect(redis.hget('vmpooler__boot__' + Date.today.to_s, pool + ':' + vm)).to_not be_nil
+        # TODO Should we inspect the value to see if it's valid?
+      end
 
+      it 'should not determine boot timespan if clone start time not set' do
+        expect(redis.hget('vmpooler__boot__' + Date.today.to_s, pool + ':' + vm)).to be_nil
         subject.move_pending_vm_to_ready(vm, pool, host)
+        expect(redis.hget('vmpooler__boot__' + Date.today.to_s, pool + ':' + vm)).to eq("") # Possible implementation bug here. Should still be nil here
+      end
+
+      it 'should raise error if clone start time is not parsable' do
+        redis.hset("vmpooler__vm__#{vm}", 'clone','iamnotparsable_asdate')
+        expect{subject.move_pending_vm_to_ready(vm, pool, host)}.to raise_error(/iamnotparsable_asdate/)
       end
     end
   end

From 2475f55103790299a2d552357a0fb537c750b4bb Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:07:57 -0800
Subject: [PATCH 09/35] (POOLER-73) Add spec tests for check_ready_vm

Add spec tests for check_ready_vm
---
 spec/unit/pool_manager_spec.rb | 125 +++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 366579235..aaa06d31a 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -257,6 +257,131 @@
     end
   end
 
+  describe '#check_ready_vm' do
+    let(:vsphere) { double('vsphere') }
+    let(:ttl) { 0 }
+
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:config:
+  vm_checktime: 15
+
+EOT
+      )
+    }
+
+    before(:each) do
+      expect(Thread).to receive(:new).and_yield
+      create_ready_vm(pool,vm)
+    end
+
+    it 'should raise an error if a TTL above zero is specified' do
+      expect { subject.check_ready_vm(vm,pool,5,vsphere) }.to raise_error(NameError) # This is an implementation bug
+    end
+
+    context 'a VM that does not need to be checked' do
+      it 'should do nothing' do
+        redis.hset("vmpooler__vm__#{vm}", 'check',Time.now.to_s)
+        subject.check_ready_vm(vm, pool, ttl, vsphere)
+      end
+    end
+
+    context 'a VM that does not exist' do
+      before do
+        allow(vsphere).to receive(:find_vm).and_return(nil)
+      end
+
+      it 'should set the current check timestamp' do
+        allow(subject).to receive(:open_socket)
+        expect(redis.hget("vmpooler__vm__#{vm}", 'check')).to be_nil
+        subject.check_ready_vm(vm, pool, ttl, vsphere)
+        expect(redis.hget("vmpooler__vm__#{vm}", 'check')).to_not be_nil
+      end
+
+      it 'should log a message' do
+        expect(logger).to receive(:log).with('s', "[!] [#{pool}] '#{vm}' not found in vCenter inventory, removed from 'ready' queue")
+        allow(subject).to receive(:open_socket)
+        subject.check_ready_vm(vm, pool, ttl, vsphere)
+      end
+
+      it 'should remove the VM from the ready queue' do
+        allow(subject).to receive(:open_socket)
+        expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(true)
+        subject.check_ready_vm(vm, pool, ttl, vsphere)
+        expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(false)
+      end
+    end
+
+    context 'a VM that needs to be checked' do
+      before(:each) do
+        redis.hset("vmpooler__vm__#{vm}", 'check',Date.new(2001,1,1).to_s)
+
+        allow(host).to receive(:summary).and_return( double('summary') )
+        allow(host).to receive_message_chain(:summary, :guest).and_return( double('guest') )
+        allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return (vm)
+        
+        allow(vsphere).to receive(:find_vm).and_return(host)
+      end
+
+      context 'and is ready' do
+        before(:each) do
+          allow(host).to receive(:runtime).and_return( double('runtime') )
+          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn')
+          allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return (vm)
+          allow(subject).to receive(:open_socket).with(vm).and_return(nil)
+        end
+
+        it 'should only set the next check interval' do
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+        end
+      end
+
+      context 'but turned off and name mismatch' do
+        before(:each) do
+          allow(host).to receive(:runtime).and_return( double('runtime') )
+          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOff')
+          allow(host).to receive_message_chain(:summary, :guest, :hostName).and_return ('')
+          allow(subject).to receive(:open_socket).with(vm).and_raise(SocketError,'getaddrinfo: No such host is known')
+        end
+
+        it 'should move the VM to the completed queue multiple times' do
+          # There is an implementation bug which attempts the move multiple times
+          expect(redis).to receive(:smove).with("vmpooler__ready__#{pool}", "vmpooler__completed__#{pool}", vm).at_least(2).times
+
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+        end
+
+        it 'should move the VM to the completed queue' do
+          expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(true)
+          expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(false)
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+          expect(redis.sismember("vmpooler__ready__#{pool}", vm)).to be(false)
+          expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(true)
+        end
+
+        it 'should log messages about being powered off, name mismatch and removed from ready queue' do
+          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' appears to be powered off, removed from 'ready' queue")
+          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' has mismatched hostname, removed from 'ready' queue")
+
+         # There is an implementation bug which attempts the move multiple times however
+         # as the VM is no longer in the ready queue, redis also throws an error
+         expect(logger).to receive(:log).with("d", "[!] [#{pool}] '#{vm}' is unreachable, and failed to remove from 'ready' queue")
+
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+        end
+
+        it 'should log a message if it fails to move queues' do
+          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' appears to be powered off, removed from 'ready' queue")
+          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' has mismatched hostname, removed from 'ready' queue")
+          expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' is unreachable, and failed to remove from 'ready' queue")
+
+          subject.check_ready_vm(vm, pool, ttl, vsphere)
+        end
+      end
+    end
+  end
+
   describe '#_check_running_vm' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From 58175dec472d79559fd22cee6bda844e17eb0331 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:08:42 -0800
Subject: [PATCH 10/35] (POOLER-73) Add spec tests for check_running_vm

Add spec tests for check_running_vm
---
 spec/unit/pool_manager_spec.rb | 56 ++++++++++++++++++++++++++--------
 1 file changed, 44 insertions(+), 12 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index aaa06d31a..8dc0d4c74 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -382,6 +382,22 @@
     end
   end
 
+  describe '#check_running_vm' do
+    let(:vsphere) { double('vsphere') }
+    let (:ttl) { 5 }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'calls _check_running_vm' do
+      expect(Thread).to receive(:new).and_yield
+      expect(subject).to receive(:_check_running_vm).with(vm, pool, ttl, vsphere)
+
+      subject.check_running_vm(vm, pool, ttl, vsphere)
+    end
+  end
+
   describe '#_check_running_vm' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }
@@ -391,10 +407,15 @@
       $vsphere = vsphere
     end
 
-    it 'does nothing with nil host' do
+    before(:each) do
+      create_running_vm(pool,vm)
+    end
+
+    it 'does nothing with a missing VM' do
       allow(vsphere).to receive(:find_vm).and_return(nil)
-      expect(redis).not_to receive(:smove)
+      expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
       subject._check_running_vm(vm, pool, timeout, vsphere)
+      expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
     end
 
     context 'valid host' do
@@ -404,24 +425,35 @@
         allow(vsphere).to receive(:find_vm).and_return vm_host
         allow(vm_host).to receive(:runtime).and_return true
         allow(vm_host).to receive_message_chain(:runtime, :powerState).and_return 'poweredOff'
-
-        expect(redis).to receive(:hget)
-        expect(redis).not_to receive(:smove)
         expect(logger).not_to receive(:log).with('d', "[!] [#{pool}] '#{vm}' appears to be powered off or dead")
-
+        expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
         subject._check_running_vm(vm, pool, timeout, vsphere)
+        expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
       end
 
-      it 'moves vm when poweredOn, but past TTL' do
+      it 'should not move VM if it has no checkout time' do
         allow(vsphere).to receive(:find_vm).and_return vm_host
-        allow(vm_host).to receive(:runtime).and_return true
-        allow(vm_host).to receive_message_chain(:runtime, :powerState).and_return 'poweredOn'
+        expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
+        subject._check_running_vm(vm, pool, 0, vsphere)
+        expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
+      end
 
-        expect(redis).to receive(:hget).with('vmpooler__active__pool1', 'vm1').and_return((Time.now - timeout*60*60).to_s)
-        expect(redis).to receive(:smove)
-        expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' reached end of TTL after #{timeout} hours")
+      it 'should not move VM if TTL is zero' do
+        allow(vsphere).to receive(:find_vm).and_return vm_host
+        redis.hset("vmpooler__active__#{pool}", vm,(Time.now - timeout*60*60).to_s)
+        expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
+        subject._check_running_vm(vm, pool, 0, vsphere)
+        expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
+      end
 
+      it 'should move VM when past TTL' do
+        allow(vsphere).to receive(:find_vm).and_return vm_host
+        redis.hset("vmpooler__active__#{pool}", vm,(Time.now - timeout*60*60).to_s)
+        expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(true)
+        expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(false)
         subject._check_running_vm(vm, pool, timeout, vsphere)
+        expect(redis.sismember("vmpooler__running__#{pool}", vm)).to be(false)
+        expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(true)
       end
     end
   end

From cb4e1400aa45f5d9781f62c028005782c0208ac3 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:09:56 -0800
Subject: [PATCH 11/35] (POOLER-73) Modify spec tests for _check_running_vm

Modify spec tests for _check_running_vm
---
 spec/unit/pool_manager_spec.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 8dc0d4c74..34838d4a1 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -399,12 +399,10 @@
   end
 
   describe '#_check_running_vm' do
-    let(:pool_helper) { double('pool') }
-    let(:vsphere) { {pool => pool_helper} }
+    let(:vsphere) { double('vsphere') }
 
     before do
       expect(subject).not_to be_nil
-      $vsphere = vsphere
     end
 
     before(:each) do
@@ -421,7 +419,9 @@
     context 'valid host' do
       let(:vm_host) { double('vmhost') }
 
-      it 'does not move vm when not poweredOn' do
+     it 'should not move VM when not poweredOn' do
+        # I'm not sure this test is useful.  There is no codepath
+        # in _check_running_vm that looks at Power State
         allow(vsphere).to receive(:find_vm).and_return vm_host
         allow(vm_host).to receive(:runtime).and_return true
         allow(vm_host).to receive_message_chain(:runtime, :powerState).and_return 'poweredOff'

From 9d8b792c4a620671fc7bfa94c8c24e1ddf8ebc29 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:10:38 -0800
Subject: [PATCH 12/35] (POOLER-73) Add spec tests for move_vm_queue

Add spec tests for move_vm_queue
---
 spec/unit/pool_manager_spec.rb | 39 ++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 34838d4a1..43b954181 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -11,6 +11,7 @@
   let(:vm) { 'vm1' }
   let(:timeout) { 5 }
   let(:host) { double('host') }
+  let(:token) { 'token1234'}
 
   subject { Vmpooler::PoolManager.new(config, logger, redis, metrics) }
 
@@ -458,6 +459,44 @@
     end
   end
 
+  describe '#move_vm_queue' do
+    let(:queue_from) { 'pending' }
+    let(:queue_to) { 'completed' }
+    let(:message) { 'message' }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      create_pending_vm(pool, vm, token)
+    end
+
+    it 'VM should be in the "from queue" before the move' do
+      expect(redis.sismember("vmpooler__#{queue_from}__#{pool}",vm))
+    end
+
+    it 'VM should not be in the "from queue" after the move' do
+      subject.move_vm_queue(pool, vm, queue_from, queue_to, message)
+      expect(!redis.sismember("vmpooler__#{queue_from}__#{pool}",vm))
+    end
+
+    it 'VM should not be in the "to queue" before the move' do
+      expect(!redis.sismember("vmpooler__#{queue_to}__#{pool}",vm))
+    end
+
+    it 'VM should be in the "to queue" after the move' do
+      subject.move_vm_queue(pool, vm, queue_from, queue_to, message)
+      expect(redis.sismember("vmpooler__#{queue_to}__#{pool}",vm))
+    end
+
+    it 'should log a message' do
+      allow(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' #{message}")
+
+      subject.move_vm_queue(pool, vm, queue_from, queue_to, message)
+    end
+  end
+
   describe '#move_running_to_completed' do
     before do
       expect(subject).not_to be_nil

From 434ab546a2e5ffcc71bc96d0ce67905061e4955c Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:12:20 -0800
Subject: [PATCH 13/35] (POOLER-73) Add spec tests for clone_vm

Add spec tests for clone_vm
---
 spec/unit/pool_manager_spec.rb | 265 +++++++++++++++++++++++++++++++++
 1 file changed, 265 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 43b954181..287c330d3 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -517,6 +517,271 @@
     end
   end
 
+  describe '#clone_vm' do
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      expect(Thread).to receive(:new).and_yield
+    end
+
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:config:
+  prefix: "prefix"
+:vsphere:
+  username: "vcenter_user"
+EOT
+      )
+    }
+
+    let (:folder) { 'vmfolder' }
+    let (:folder_object) { double('folder_object') }
+    let (:template_name) { pool }
+    let (:template) { "template/#{template_name}" }
+    let (:datastore) { 'datastore' }
+    let (:target) { 'clone_target' }
+    let (:vsphere) { double('vsphere') }
+    let (:template_folder_object) { double('template_folder_object') }
+    let (:template_vm_object) { double('template_vm_object') }
+    let (:clone_task) { double('clone_task') }
+
+    context 'no template specified' do
+      it 'should raise an error' do
+  
+        expect{subject.clone_vm(nil,folder,datastore,target,vsphere)}.to raise_error(/Please provide a full path to the template/)
+      end
+
+      it 'should log a message' do
+        expect(logger).to receive(:log).with('s', "[!] [] '' failed while preparing to clone with an error: Please provide a full path to the template")
+
+        expect{subject.clone_vm(nil,folder,datastore,target,vsphere)}.to raise_error(RuntimeError)
+      end
+    end
+
+    context 'a template with no forward slash in the string' do
+      it 'should raise an error' do
+  
+        expect{subject.clone_vm('vm1',folder,datastore,target,vsphere)}.to raise_error(/Please provide a full path to the template/)
+      end
+
+      it 'should log a message' do
+        expect(logger).to receive(:log).with('s', "[!] [] '' failed while preparing to clone with an error: Please provide a full path to the template")
+
+        expect{subject.clone_vm('vm1',folder,datastore,target,vsphere)}.to raise_error(RuntimeError)
+      end
+    end
+
+    # Note - It is impossible to get into the following code branch
+    # ...
+    # if vm['template'].length == 0
+    #   fail "Unable to find template '#{vm['template']}'!"
+    # end
+    # ...
+
+    context "Template name does not match pool name (Implementation Bug)" do
+      let (:template_name) { 'template_vm' }
+
+      # The implementaion of clone_vm incorrectly uses the VM Template name instead of the pool name.  The VM Template represents the
+      # name of the VM to clone in vSphere whereas pool is the name of the pool in Pooler.  The tests below document the behaviour of
+      # clone_vm if the Template and Pool name differ.  It is expected that these test will fail once this bug is removed.
+
+      context 'a valid template' do
+        before(:each) do
+          expect(template_folder_object).to receive(:find).with(template_name).and_return(template_vm_object)
+          expect(vsphere).to receive(:find_folder).with('template').and_return(template_folder_object)
+        end
+
+        context 'with no errors during cloning' do
+          before(:each) do
+            expect(vsphere).to receive(:find_least_used_host).with(target).and_return('least_used_host')
+            expect(vsphere).to receive(:find_datastore).with(datastore).and_return('datastore')
+            expect(vsphere).to receive(:find_folder).with('vmfolder').and_return(folder_object)
+            expect(template_vm_object).to receive(:CloneVM_Task).and_return(clone_task)
+            expect(clone_task).to receive(:wait_for_completion)
+            expect(metrics).to receive(:timing).with(/clone\./,/0/)
+          end
+
+          it 'should create a cloning VM' do
+            expect(logger).to receive(:log).at_least(:once)
+            expect(redis.scard("vmpooler__pending__#{pool}")).to eq(0)
+
+            subject.clone_vm(template,folder,datastore,target,vsphere)
+
+            expect(redis.scard("vmpooler__pending__#{template_name}")).to eq(1)
+            # Get the new VM Name from the pending pool queue as it should be the only entry
+            vm_name = redis.smembers("vmpooler__pending__#{template_name}")[0]
+            expect(redis.hget("vmpooler__vm__#{vm_name}", 'clone')).to_not be_nil
+            expect(redis.hget("vmpooler__vm__#{vm_name}", 'template')).to eq(template_name)
+            expect(redis.hget("vmpooler__clone__#{Date.today.to_s}", "#{template_name}:#{vm_name}")).to_not be_nil
+            expect(redis.hget("vmpooler__vm__#{vm_name}", 'clone_time')).to_not be_nil
+          end
+
+          it 'should log a message that is being cloned from a template' do
+            expect(logger).to receive(:log).with('d',/\[ \] \[#{template_name}\] '(.+)' is being cloned from '#{template_name}'/)
+            allow(logger).to receive(:log)
+
+            subject.clone_vm(template,folder,datastore,target,vsphere)
+          end
+
+          it 'should log a message that it completed being cloned' do
+            expect(logger).to receive(:log).with('s',/\[\+\] \[#{template_name}\] '(.+)' cloned from '#{template_name}' in [0-9.]+ seconds/)
+            allow(logger).to receive(:log)
+
+            subject.clone_vm(template,folder,datastore,target,vsphere)
+          end
+        end
+
+        # An error can be cause by the following configuration errors:
+        # - Missing or invalid datastore
+        # - Missing or invalid clone target
+        # also any runtime errors during the cloning process
+        # https://www.vmware.com/support/developer/converter-sdk/conv50_apireference/vim.VirtualMachine.html#clone
+        context 'with an error during cloning' do
+          before(:each) do
+            expect(vsphere).to receive(:find_least_used_host).with(target).and_return('least_used_host')
+            expect(vsphere).to receive(:find_datastore).with(datastore).and_return(nil)
+            expect(vsphere).to receive(:find_folder).with('vmfolder').and_return(folder_object)
+            expect(template_vm_object).to receive(:CloneVM_Task).and_return(clone_task)
+            expect(clone_task).to receive(:wait_for_completion).and_raise(RuntimeError,'SomeError')
+            expect(metrics).to receive(:timing).with(/clone\./,/0/).exactly(0).times
+
+          end
+
+          it 'should raise an error within the Thread' do
+            expect(logger).to receive(:log).at_least(:once)
+            expect{subject.clone_vm(template,folder,datastore,target,vsphere)}.to raise_error(/SomeError/)
+          end
+
+          it 'should log a message that is being cloned from a template' do
+            expect(logger).to receive(:log).with('d',/\[ \] \[#{template_name}\] '(.+)' is being cloned from '#{template_name}'/)
+            allow(logger).to receive(:log)
+
+            # Swallow the error
+            begin
+              subject.clone_vm(template,folder,datastore,target,vsphere)
+            rescue
+            end
+          end
+
+          it 'should log messages that the clone failed' do
+            expect(logger).to receive(:log).with('s', /\[!\] \[#{template_name}\] '(.+)' clone failed with an error: SomeError/)
+            expect(logger).to receive(:log).with('s', /\[!\] \[#{template_name}\] '(.+)' failed while preparing to clone with an error: SomeError/)
+            allow(logger).to receive(:log)
+
+            # Swallow the error
+            begin
+              subject.clone_vm(template,folder,datastore,target,vsphere)
+            rescue
+            end
+          end
+        end
+      end
+    end
+
+    context 'a valid template' do
+      before(:each) do
+        expect(template_folder_object).to receive(:find).with(template_name).and_return(template_vm_object)
+        expect(vsphere).to receive(:find_folder).with('template').and_return(template_folder_object)
+      end
+
+      context 'with no errors during cloning' do
+        before(:each) do
+          expect(vsphere).to receive(:find_least_used_host).with(target).and_return('least_used_host')
+          expect(vsphere).to receive(:find_datastore).with(datastore).and_return('datastore')
+          expect(vsphere).to receive(:find_folder).with('vmfolder').and_return(folder_object)
+          expect(template_vm_object).to receive(:CloneVM_Task).and_return(clone_task)
+          expect(clone_task).to receive(:wait_for_completion)
+          expect(metrics).to receive(:timing).with(/clone\./,/0/)
+        end
+
+        it 'should create a cloning VM' do
+          expect(logger).to receive(:log).at_least(:once)
+          expect(redis.scard("vmpooler__pending__#{pool}")).to eq(0)
+
+          subject.clone_vm(template,folder,datastore,target,vsphere)
+
+          expect(redis.scard("vmpooler__pending__#{pool}")).to eq(1)
+          # Get the new VM Name from the pending pool queue as it should be the only entry
+          vm_name = redis.smembers("vmpooler__pending__#{pool}")[0]
+          expect(redis.hget("vmpooler__vm__#{vm_name}", 'clone')).to_not be_nil
+          expect(redis.hget("vmpooler__vm__#{vm_name}", 'template')).to eq(template_name)
+          expect(redis.hget("vmpooler__clone__#{Date.today.to_s}", "#{pool}:#{vm_name}")).to_not be_nil
+          expect(redis.hget("vmpooler__vm__#{vm_name}", 'clone_time')).to_not be_nil
+        end
+
+        it 'should decrement the clone tasks counter' do
+          redis.incr('vmpooler__tasks__clone')
+          redis.incr('vmpooler__tasks__clone')
+          expect(redis.get('vmpooler__tasks__clone')).to eq('2')
+          subject.clone_vm(template,folder,datastore,target,vsphere)
+          expect(redis.get('vmpooler__tasks__clone')).to eq('1')
+        end
+
+        it 'should log a message that is being cloned from a template' do
+          expect(logger).to receive(:log).with('d',/\[ \] \[#{pool}\] '(.+)' is being cloned from '#{template_name}'/)
+          allow(logger).to receive(:log)
+
+          subject.clone_vm(template,folder,datastore,target,vsphere)
+        end
+
+        it 'should log a message that it completed being cloned' do
+          expect(logger).to receive(:log).with('s',/\[\+\] \[#{pool}\] '(.+)' cloned from '#{template_name}' in [0-9.]+ seconds/)
+          allow(logger).to receive(:log)
+
+          subject.clone_vm(template,folder,datastore,target,vsphere)
+        end
+      end
+
+      # An error can be cause by the following configuration errors:
+      # - Missing or invalid datastore
+      # - Missing or invalid clone target
+      # also any runtime errors during the cloning process
+      # https://www.vmware.com/support/developer/converter-sdk/conv50_apireference/vim.VirtualMachine.html#clone
+      context 'with an error during cloning' do
+        before(:each) do
+          expect(vsphere).to receive(:find_least_used_host).with(target).and_return('least_used_host')
+          expect(vsphere).to receive(:find_datastore).with(datastore).and_return(nil)
+          expect(vsphere).to receive(:find_folder).with('vmfolder').and_return(folder_object)
+          expect(template_vm_object).to receive(:CloneVM_Task).and_return(clone_task)
+          expect(clone_task).to receive(:wait_for_completion).and_raise(RuntimeError,'SomeError')
+          expect(metrics).to receive(:timing).with(/clone\./,/0/).exactly(0).times
+
+        end
+
+        it 'should raise an error within the Thread' do
+          expect(logger).to receive(:log).at_least(:once)
+          expect{subject.clone_vm(template,folder,datastore,target,vsphere)}.to raise_error(/SomeError/)
+        end
+
+        it 'should log a message that is being cloned from a template' do
+          expect(logger).to receive(:log).with('d',/\[ \] \[#{pool}\] '(.+)' is being cloned from '#{template_name}'/)
+          allow(logger).to receive(:log)
+
+          # Swallow the error
+          begin
+            subject.clone_vm(template,folder,datastore,target,vsphere)
+          rescue
+          end
+        end
+
+        it 'should log messages that the clone failed' do
+          expect(logger).to receive(:log).with('s', /\[!\] \[#{pool}\] '(.+)' clone failed with an error: SomeError/)
+          expect(logger).to receive(:log).with('s', /\[!\] \[#{pool}\] '(.+)' failed while preparing to clone with an error: SomeError/)
+          allow(logger).to receive(:log)
+
+          # Swallow the error
+          begin
+            subject.clone_vm(template,folder,datastore,target,vsphere)
+          rescue
+          end
+        end
+      end
+    end
+  end
+
   describe '#_check_pool' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From 14c091383cc007122820713e17ffd255a18588e0 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Wed, 8 Feb 2017 17:13:12 -0800
Subject: [PATCH 14/35] (POOLER-73) Add spec tests for destroy_vm

Add spec tests for destroy_vm
---
 spec/unit/pool_manager_spec.rb | 138 +++++++++++++++++++++++++++++++++
 1 file changed, 138 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 287c330d3..854c83841 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -782,6 +782,144 @@
     end
   end
 
+  describe "#destroy_vm" do
+    let (:vsphere) { double('vsphere') }
+
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:redis:
+  data_ttl: 168
+EOT
+      )
+    }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      expect(Thread).to receive(:new).and_yield
+
+      create_completed_vm(vm,pool,true)
+    end
+
+    context 'when redis data_ttl is not specified in the configuration' do
+      let(:config) {
+        YAML.load(<<-EOT
+---
+:redis:
+  "key": "value"
+EOT
+      )
+      }
+
+      before(:each) do
+        expect(vsphere).to receive(:find_vm).and_return(nil)
+      end
+
+      it 'should call redis expire with 0' do
+        expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to_not be_nil
+        subject.destroy_vm(vm,pool,vsphere)
+        expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to be_nil
+      end
+    end
+
+    context 'when there is no redis section in the configuration' do
+      let(:config) {}
+      
+      it 'should raise an error' do
+        expect{ subject.destroy_vm(vm,pool,vsphere) }.to raise_error(NoMethodError)
+      end
+    end
+
+    context 'when a VM does not exist' do
+      before(:each) do
+        expect(vsphere).to receive(:find_vm).and_return(nil)
+      end
+
+      it 'should not call any vsphere methods' do
+        subject.destroy_vm(vm,pool,vsphere)
+      end
+    end
+
+    context 'when a VM exists' do
+      let (:destroy_task) { double('destroy_task') }
+      let (:poweroff_task) { double('poweroff_task') }
+
+      before(:each) do
+        expect(vsphere).to receive(:find_vm).and_return(host)
+        allow(host).to receive(:runtime).and_return(true)
+      end
+
+      context 'and an error occurs during destroy' do
+        before(:each) do
+          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOff')
+          expect(host).to receive(:Destroy_Task).and_return(destroy_task)
+          expect(destroy_task).to receive(:wait_for_completion).and_raise(RuntimeError,'DestroyFailure')
+          expect(metrics).to receive(:timing).exactly(0).times
+        end
+
+        it 'should raise an error in the thread' do
+          expect { subject.destroy_vm(vm,pool,vsphere) }.to raise_error(/DestroyFailure/)
+        end
+      end
+
+      context 'and an error occurs during power off' do
+        before(:each) do
+          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn')
+          expect(host).to receive(:PowerOffVM_Task).and_return(poweroff_task)
+          expect(poweroff_task).to receive(:wait_for_completion).and_raise(RuntimeError,'PowerOffFailure')
+          expect(logger).to receive(:log).with('d', "[ ] [#{pool}] '#{vm}' is being shut down")
+          expect(metrics).to receive(:timing).exactly(0).times
+        end
+
+        it 'should raise an error in the thread' do
+          expect { subject.destroy_vm(vm,pool,vsphere) }.to raise_error(/PowerOffFailure/)
+        end
+      end
+
+      context 'and is powered off' do
+        before(:each) do
+          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOff')
+          expect(host).to receive(:Destroy_Task).and_return(destroy_task)
+          expect(destroy_task).to receive(:wait_for_completion)
+          expect(metrics).to receive(:timing).with("destroy.#{pool}", /0/)
+        end
+
+        it 'should log a message the VM was destroyed' do
+          expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/)
+          subject.destroy_vm(vm,pool,vsphere)
+        end
+      end
+
+      context 'and is powered on' do
+        before(:each) do
+          allow(host).to receive_message_chain(:runtime, :powerState).and_return('poweredOn')
+          expect(host).to receive(:Destroy_Task).and_return(destroy_task)
+          expect(host).to receive(:PowerOffVM_Task).and_return(poweroff_task)
+          expect(poweroff_task).to receive(:wait_for_completion)
+          expect(destroy_task).to receive(:wait_for_completion)
+          expect(metrics).to receive(:timing).with("destroy.#{pool}", /0/)
+        end
+
+        it 'should log a message the VM is being shutdown' do
+          expect(logger).to receive(:log).with('d', "[ ] [#{pool}] '#{vm}' is being shut down")
+          allow(logger).to receive(:log)
+
+          subject.destroy_vm(vm,pool,vsphere)
+        end
+
+        it 'should log a message the VM was destroyed' do
+         expect(logger).to receive(:log).with('s', /\[-\] \[#{pool}\] '#{vm}' destroyed in [0-9.]+ seconds/)
+          allow(logger).to receive(:log)
+
+          subject.destroy_vm(vm,pool,vsphere)
+        end
+      end
+    end
+  end
+
   describe '#_check_pool' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From e342cfe56ac6a356d8e0aee7eaba7391ad2e3a56 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 10 Feb 2017 13:08:15 -0800
Subject: [PATCH 15/35] (POOLER-73) Add spec tests for get_vm_host_info

Add spec tests for get_vm_host_info
---
 spec/unit/pool_manager_spec.rb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 854c83841..d89b82bd7 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -920,6 +920,22 @@
     end
   end
 
+  describe "#get_vm_host_info" do
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    let(:vm_object) { double('vm_object') }
+    let(:parent_host) { double('parent_host') }
+
+    it 'should return an array with host information' do
+      expect(vm_object).to receive_message_chain(:summary, :runtime, :host).and_return(parent_host)
+      expect(parent_host).to receive(:name).and_return('vmhostname')
+
+      expect(subject.get_vm_host_info(vm_object)).to eq([parent_host,'vmhostname'])
+    end
+  end
+
   describe '#_check_pool' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From 5e46ace584b74d216cdf445704a81dda763b738b Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 10 Feb 2017 13:10:55 -0800
Subject: [PATCH 16/35] (POOLER-73) Add spec tests for execute!

Add spec tests for execute!

Previously the execute! method would execute the loop indefinitely as it did not
have a terminating condition.  This made it impossible to test.  This commit
modifies the execute! method so that it can take a maxloop and delay parameter
so that it can be tested.
---
 lib/vmpooler/pool_manager.rb   |  10 +-
 spec/unit/pool_manager_spec.rb | 193 +++++++++++++++++++++++++++++++++
 2 files changed, 201 insertions(+), 2 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 4f5571100..f3280ab84 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -714,7 +714,7 @@ def _check_pool(pool, vsphere)
       raise
     end
 
-    def execute!
+   def execute!(maxloop = 0, loop_delay = 1)
       $logger.log('d', 'starting vmpooler')
 
       # Clear out the tasks manager, as we don't know about any tasks at this point
@@ -722,6 +722,7 @@ def execute!
       # Clear out vmpooler__migrations since stale entries may be left after a restart
       $redis.del('vmpooler__migration')
 
+      loop_count = 1
       loop do
         if ! $threads['disk_manager']
           check_disk_queue
@@ -746,7 +747,12 @@ def execute!
           end
         end
 
-        sleep(1)
+        sleep(loop_delay)
+
+        unless maxloop.zero?
+          break if loop_count >= maxloop
+          loop_count = loop_count + 1
+        end
       end
     end
   end
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index d89b82bd7..d6941a03b 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -2,6 +2,13 @@
 require 'time'
 require 'mock_redis'
 
+# Custom RSpec :Matchers
+
+# Match a Hashtable['name'] is an expected value
+RSpec::Matchers.define :a_pool_with_name_of do |value|
+  match { |actual| actual['name'] == value }
+end
+
 describe 'Pool Manager' do
   let(:logger) { MockLogger.new }
   let(:redis) { MockRedis.new }
@@ -936,6 +943,192 @@
     end
   end
 
+  describe "#execute!" do
+    let(:threads) {{}}
+
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:pools:
+  - name: #{pool}
+EOT
+      )
+    }
+
+    let(:thread) { double('thread') }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    context 'on startup' do
+      before(:each) do
+        allow(subject).to receive(:check_disk_queue)
+        allow(subject).to receive(:check_snapshot_queue)
+        allow(subject).to receive(:check_pool)
+        expect(logger).to receive(:log).with('d', 'starting vmpooler')
+      end
+
+      it 'should set clone tasks to zero' do
+        redis.set('vmpooler__tasks__clone', 1)
+        subject.execute!(1,0)
+        expect(redis.get('vmpooler__tasks__clone')).to eq('0')
+      end
+
+      it 'should clear migration tasks' do
+        redis.set('vmpooler__migration', 1)
+        subject.execute!(1,0)
+        expect(redis.get('vmpooler__migration')).to be_nil
+      end
+
+      it 'should run the check_disk_queue method' do
+        expect(subject).to receive(:check_disk_queue)
+
+        subject.execute!(1,0)
+      end
+
+      it 'should run the check_snapshot_queue method' do
+        expect(subject).to receive(:check_snapshot_queue)
+
+        subject.execute!(1,0)
+      end
+
+      it 'should check the pools in the config' do
+        expect(subject).to receive(:check_pool).with(a_pool_with_name_of(pool))
+
+        subject.execute!(1,0)
+      end
+    end
+
+    context 'with dead disk_manager thread' do
+      before(:each) do
+        allow(subject).to receive(:check_snapshot_queue)
+        allow(subject).to receive(:check_pool)
+        expect(logger).to receive(:log).with('d', 'starting vmpooler')
+      end
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+      end
+
+      it 'should run the check_disk_queue method and log a message' do
+        expect(thread).to receive(:alive?).and_return(false)
+        expect(subject).to receive(:check_disk_queue)
+        expect(logger).to receive(:log).with('d', "[!] [disk_manager] worker thread died, restarting")
+        $threads['disk_manager'] = thread
+
+        subject.execute!(1,0)
+      end
+    end
+
+    context 'with dead snapshot_manager thread' do
+      before(:each) do
+        allow(subject).to receive(:check_disk_queue)
+        allow(subject).to receive(:check_pool)
+        expect(logger).to receive(:log).with('d', 'starting vmpooler')
+      end
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+      end
+
+      it 'should run the check_snapshot_queue method and log a message' do
+        expect(thread).to receive(:alive?).and_return(false)
+        expect(subject).to receive(:check_snapshot_queue)
+        expect(logger).to receive(:log).with('d', "[!] [snapshot_manager] worker thread died, restarting")
+        $threads['snapshot_manager'] = thread
+
+        subject.execute!(1,0)
+      end
+    end
+
+    context 'with dead pool thread' do
+      before(:each) do
+        allow(subject).to receive(:check_disk_queue)
+        allow(subject).to receive(:check_snapshot_queue)
+        expect(logger).to receive(:log).with('d', 'starting vmpooler')
+      end
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+      end
+
+      it 'should run the check_pool method and log a message' do
+        expect(thread).to receive(:alive?).and_return(false)
+        expect(subject).to receive(:check_pool).with(a_pool_with_name_of(pool))
+        expect(logger).to receive(:log).with('d', "[!] [#{pool}] worker thread died, restarting")
+        $threads[pool] = thread
+
+        subject.execute!(1,0)
+      end
+    end
+
+    context 'delays between loops' do
+      let(:maxloop) { 2 }
+      let(:loop_delay) { 1 }
+      # Note a maxloop of zero can not be tested as it never terminates
+      before(:each) do
+  
+        allow(subject).to receive(:check_disk_queue)
+        allow(subject).to receive(:check_snapshot_queue)
+        allow(subject).to receive(:check_pool)
+      end
+
+      it 'when a non-default loop delay is specified' do
+        start_time = Time.now
+        subject.execute!(maxloop,loop_delay)
+        finish_time = Time.now
+
+        # Use a generous delta to take into account various CPU load etc.
+        expect(finish_time - start_time).to be_within(0.75).of(maxloop * loop_delay)
+      end
+    end
+
+    context 'loops specified number of times (5)' do
+      let(:maxloop) { 5 }
+      # Note a maxloop of zero can not be tested as it never terminates
+      before(:each) do
+        end
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+      end
+
+      it 'should run startup tasks only once' do
+        allow(subject).to receive(:check_disk_queue)
+        allow(subject).to receive(:check_snapshot_queue)
+        allow(subject).to receive(:check_pool)
+
+        subject.execute!(maxloop,0)
+      end
+
+      it 'should run per thread tasks 5 times when threads are not remembered' do
+        expect(subject).to receive(:check_disk_queue).exactly(maxloop).times
+        expect(subject).to receive(:check_snapshot_queue).exactly(maxloop).times
+        expect(subject).to receive(:check_pool).exactly(maxloop).times
+
+        subject.execute!(maxloop,0)
+      end
+
+      it 'should not run per thread tasks when threads are alive' do
+        expect(subject).to receive(:check_disk_queue).exactly(0).times
+        expect(subject).to receive(:check_snapshot_queue).exactly(0).times
+        expect(subject).to receive(:check_pool).exactly(0).times
+
+        allow(thread).to receive(:alive?).and_return(true)
+        $threads[pool] = thread
+        $threads['disk_manager'] = thread
+        $threads['snapshot_manager'] = thread
+
+        subject.execute!(maxloop,0)
+      end
+    end
+  end
+
   describe '#_check_pool' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From daad5c708692c794dd65565ca5003cf5b518a392 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 10 Feb 2017 13:18:13 -0800
Subject: [PATCH 17/35] (POOLER-73) Add spec tests for check_pool

Add spec tests for check_pool

Previously the check_pool method would execute the loop indefinitely as it did not
have a terminating condition.  This made it impossible to test.  This commit
modifies the check_pool method so that it can take a maxloop and delay parameter
so that it can be tested.
---
 lib/vmpooler/pool_manager.rb   |  10 ++-
 spec/unit/pool_manager_spec.rb | 108 +++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+), 2 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index f3280ab84..2892adc89 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -546,15 +546,21 @@ def migrate_vm_and_record_timing(vm_object, vm_name, pool, host, source_host_nam
       finish
     end
 
-    def check_pool(pool)
+    def check_pool(pool,maxloop = 0, loop_delay = 5)
       $logger.log('d', "[*] [#{pool['name']}] starting worker thread")
 
       $vsphere[pool['name']] ||= Vmpooler::VsphereHelper.new $config, $metrics
 
       $threads[pool['name']] = Thread.new do
+        loop_count = 1
         loop do
           _check_pool(pool, $vsphere[pool['name']])
-          sleep(5)
+          sleep(loop_delay)
+
+          unless maxloop.zero?
+            break if loop_count >= maxloop
+            loop_count = loop_count + 1
+          end
         end
       end
     end
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index d6941a03b..435464146 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1129,6 +1129,114 @@
     end
   end
 
+ describe "#check_pool" do
+    let(:threads) {{}}
+    let(:vsphere) {{}}
+
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:pools:
+  - name: #{pool}
+EOT
+      )
+    }
+
+    let(:thread) { double('thread') }
+    let(:pool_object) { config[:pools][0] }
+
+    before do
+      expect(subject).not_to be_nil
+      expect(Thread).to receive(:new).and_yield
+    end
+
+    context 'on startup' do
+      before(:each) do
+        # Note the Vmpooler::VsphereHelper is not mocked
+        allow(subject).to receive(:_check_pool)        
+        expect(logger).to receive(:log).with('d', "[*] [#{pool}] starting worker thread")
+      end
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+        $vsphere = nil
+      end
+
+      it 'should log a message the worker thread is starting' do
+        subject.check_pool(pool_object,1,0)
+      end
+
+      it 'should populate the vsphere global variable' do
+        subject.check_pool(pool_object,1,0)
+
+        expect($vsphere[pool]).to_not be_nil 
+      end
+
+      it 'should populate the threads global variable' do
+        subject.check_pool(pool_object,1,0)
+
+        # Unable to test for nil as the Thread is mocked
+        expect($threads.keys.include?(pool))
+      end
+    end
+
+    context 'delays between loops' do
+      let(:maxloop) { 2 }
+      let(:loop_delay) { 1 }
+      # Note a maxloop of zero can not be tested as it never terminates
+
+      before(:each) do
+        allow(logger).to receive(:log)
+        # Note the Vmpooler::VsphereHelper is not mocked
+        allow(subject).to receive(:_check_pool)        
+      end
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+        $vsphere = nil
+      end
+
+      it 'when a non-default loop delay is specified' do
+        start_time = Time.now
+        subject.check_pool(pool_object,maxloop,loop_delay)
+        finish_time = Time.now
+
+        # Use a generous delta to take into account various CPU load etc.
+        expect(finish_time - start_time).to be_within(0.75).of(maxloop * loop_delay)
+      end
+    end
+
+    context 'loops specified number of times (5)' do
+      let(:maxloop) { 5 }
+      # Note a maxloop of zero can not be tested as it never terminates
+      before(:each) do
+        allow(logger).to receive(:log)
+        # Note the Vmpooler::VsphereHelper is not mocked
+        allow(subject).to receive(:_check_pool)        
+      end
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+        $vsphere = nil
+      end
+
+      it 'should run startup tasks only once' do
+        expect(logger).to receive(:log).with('d', "[*] [#{pool}] starting worker thread")
+
+        subject.check_pool(pool_object,maxloop,0)
+      end
+
+      it 'should run per thread tasks 5 times' do
+        expect(subject).to receive(:_check_pool).exactly(maxloop).times
+
+        subject.check_pool(pool_object,maxloop,0)
+      end
+    end
+  end
+
   describe '#_check_pool' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From 64d2d95ab357a192691bcc5bed57d227a4f4fabc Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 11:38:38 -0800
Subject: [PATCH 18/35] (POOLER-73) Remove #move_running_to_completed tests

The tests for #move_running_to_completed are already tested in the
---
 spec/unit/pool_manager_spec.rb | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 435464146..ed2b896cb 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -504,26 +504,6 @@
     end
   end
 
-  describe '#move_running_to_completed' do
-    before do
-      expect(subject).not_to be_nil
-    end
-
-    it 'uses the pool in smove' do
-      allow(redis).to receive(:smove).with(String, String, String)
-      allow(logger).to receive(:log)
-      expect(redis).to receive(:smove).with('vmpooler__running__p1', 'vmpooler__completed__p1', 'vm1')
-      subject.move_vm_queue('p1', 'vm1', 'running', 'completed', 'msg')
-    end
-
-    it 'logs msg' do
-      allow(redis).to receive(:smove)
-      allow(logger).to receive(:log)
-      expect(logger).to receive(:log).with('d', "[!] [p1] 'vm1' a msg here")
-      subject.move_vm_queue('p1', 'vm1', 'running', 'completed', 'a msg here')
-    end
-  end
-
   describe '#clone_vm' do
     before do
       expect(subject).not_to be_nil

From 15d4dfa64aead59324c483ed209d171f2d930af7 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 12:00:34 -0800
Subject: [PATCH 19/35] (POOLER-73) Add spec tests for _check_pool

Add spec tests for _check_pool
---
 spec/unit/pool_manager_spec.rb | 558 +++++++++++++++++++++++++++++++--
 1 file changed, 533 insertions(+), 25 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index ed2b896cb..12feed2e9 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1218,37 +1218,545 @@
   end
 
   describe '#_check_pool' do
-    let(:pool_helper) { double('pool') }
-    let(:vsphere) { {pool => pool_helper} }
-    let(:config) { {
-      config: { task_limit: 10 },
-      pools: [ {'name' => 'pool1', 'size' => 5} ]
-    } }
+    # Default test fixtures will consist of;
+    #   - Empty Redis dataset
+    #   - A single pool with a pool size of zero i.e. no new VMs should be created
+    #   - Task limit of 10
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:config:
+  task_limit: 10
+:pools:
+  - name: #{pool}
+    folder: 'vm_folder'
+    size: 0
+EOT
+      )
+    }
+    let(:pool_object) { config[:pools][0] }
+    let(:vsphere) { double('vsphere') }
+    let(:new_vm) { 'newvm'}
 
     before do
       expect(subject).not_to be_nil
-      $vsphere = vsphere
-      allow(logger).to receive(:log)
-      allow(pool_helper).to receive(:find_folder)
-      allow(redis).to receive(:smembers).with('vmpooler__pending__pool1').and_return([])
-      allow(redis).to receive(:smembers).with('vmpooler__ready__pool1').and_return([])
-      allow(redis).to receive(:smembers).with('vmpooler__running__pool1').and_return([])
-      allow(redis).to receive(:smembers).with('vmpooler__completed__pool1').and_return([])
-      allow(redis).to receive(:smembers).with('vmpooler__discovered__pool1').and_return([])
-      allow(redis).to receive(:smembers).with('vmpooler__migrating__pool1').and_return([])
-      allow(redis).to receive(:set)
-      allow(redis).to receive(:get).with('vmpooler__tasks__clone').and_return(0)
-      allow(redis).to receive(:get).with('vmpooler__empty__pool1').and_return(nil)
+      allow(logger).to receive(:log).with("s", "[!] [#{pool}] is empty")
     end
 
-    context 'logging' do
-      it 'logs empty pool' do
-        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(0)
+    # INVENTORY
+    context 'Conducting inventory' do
+      before(:each) do
+        allow(subject).to receive(:migrate_vm)
+        allow(subject).to receive(:check_running_vm)
+        allow(subject).to receive(:check_ready_vm)
+        allow(subject).to receive(:check_pending_vm)
+        allow(subject).to receive(:destroy_vm)
+        allow(subject).to receive(:clone_vm)
+      end
 
-        expect(logger).to receive(:log).with('s', "[!] [pool1] is empty")
-        subject._check_pool(config[:pools][0], vsphere)
+      it 'should log an error if one occurs' do
+        expect(vsphere).to receive(:find_folder).and_raise(RuntimeError,'Mock Error')
+        expect(logger).to receive(:log).with('s', "[!] [#{pool}] _check_pool failed with an error while inspecting inventory: Mock Error")
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should log the discovery of VMs' do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([new_vm]))
+        expect(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue")
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should add undiscovered VMs to the completed queue' do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([new_vm]))
+        allow(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue")
+
+        expect(redis.sismember("vmpooler__discovered__#{pool}", new_vm)).to be(false)
+        expect(redis.sismember("vmpooler__completed__#{pool}", new_vm)).to be(false)
+
+        subject._check_pool(pool_object,vsphere)
+
+        expect(redis.sismember("vmpooler__discovered__#{pool}", new_vm)).to be(false)
+        expect(redis.sismember("vmpooler__completed__#{pool}", new_vm)).to be(true)
+      end
+
+      ['running','ready','pending','completed','discovered','migrating'].each do |queue_name|
+        it "should not discover VMs in the #{queue_name} queue" do
+          expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([new_vm]))
+
+          expect(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue").exactly(0).times
+          expect(redis.sismember("vmpooler__discovered__#{pool}", new_vm)).to be(false)
+          redis.sadd("vmpooler__#{queue_name}__#{pool}", new_vm)
+
+          subject._check_pool(pool_object,vsphere)
+
+          if queue_name == 'discovered'
+            # Discovered VMs end up in the completed queue
+            expect(redis.sismember("vmpooler__completed__#{pool}", new_vm)).to be(true)
+          else
+            expect(redis.sismember("vmpooler__#{queue_name}__#{pool}", new_vm)).to be(true)
+          end
+        end
+      end
+    end
+
+    # RUNNING
+    context 'Running VM not in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([new_vm]))
+        expect(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue")
+        create_running_vm(pool,vm,token)
+      end
+
+      it 'should not do anything' do
+        expect(subject).to receive(:check_running_vm).exactly(0).times
+
+        subject._check_pool(pool_object,vsphere)
+      end
+    end
+
+    context 'Running VM in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+        allow(subject).to receive(:check_running_vm)
+        create_running_vm(pool,vm,token)
+      end
+
+      it 'should log an error if one occurs' do
+        expect(subject).to receive(:check_running_vm).and_raise(RuntimeError,'MockError')
+        expect(logger).to receive(:log).with('d', "[!] [#{pool}] _check_pool with an error while evaluating running VMs: MockError")
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should use the VM lifetime in preference to defaults' do
+        big_lifetime = 2000
+
+        redis.hset("vmpooler__vm__#{vm}", 'lifetime',big_lifetime)
+        # The lifetime comes in as string
+        expect(subject).to receive(:check_running_vm).with(vm,pool,"#{big_lifetime}",vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should use the configuration default if the VM lifetime is not set' do
+        config[:config]['vm_lifetime'] = 50
+        expect(subject).to receive(:check_running_vm).with(vm,pool,50,vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should use a lifetime of 12 if nothing is set' do
+        expect(subject).to receive(:check_running_vm).with(vm,pool,12,vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+    end
+
+    # READY
+    context 'Ready VM not in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([new_vm]))
+        expect(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue")
+        create_ready_vm(pool,vm,token)
+      end
+
+      it 'should not do anything' do
+        expect(subject).to receive(:check_ready_vm).exactly(0).times
+
+        subject._check_pool(pool_object,vsphere)
+      end
+    end
+
+    context 'Ready VM in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+        allow(subject).to receive(:check_ready_vm)
+        create_ready_vm(pool,vm,token)
+      end
+
+      it 'should log an error if one occurs' do
+        expect(subject).to receive(:check_ready_vm).and_raise(RuntimeError,'MockError')
+        expect(logger).to receive(:log).with('d', "[!] [#{pool}] _check_pool failed with an error while evaluating ready VMs: MockError")
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should use the pool TTL if set' do
+        big_lifetime = 2000
+
+        config[:pools][0]['ready_ttl'] = big_lifetime
+        expect(subject).to receive(:check_ready_vm).with(vm,pool,big_lifetime,vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should use a pool TTL of zero if none set' do
+        expect(subject).to receive(:check_ready_vm).with(vm,pool,0,vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+    end
+
+    # PENDING
+    context 'Pending VM not in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([new_vm]))
+        expect(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue")
+        create_pending_vm(pool,vm,token)
+      end
+
+      it 'should call fail_pending_vm' do
+        expect(subject).to receive(:check_ready_vm).exactly(0).times
+        expect(subject).to receive(:fail_pending_vm).with(vm,pool,Integer,false)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+    end
+
+    context 'Pending VM in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+        allow(subject).to receive(:check_pending_vm)
+        create_pending_vm(pool,vm,token)
+      end
+
+      it 'should log an error if one occurs' do
+        expect(subject).to receive(:check_pending_vm).and_raise(RuntimeError,'MockError')
+        expect(logger).to receive(:log).with('d', "[!] [#{pool}] _check_pool failed with an error while evaluating pending VMs: MockError")
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should use the pool timeout if set' do
+        big_lifetime = 2000
+
+        config[:pools][0]['timeout'] = big_lifetime
+        expect(subject).to receive(:check_pending_vm).with(vm,pool,big_lifetime,vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should use the configuration setting if the pool timeout is not set' do
+        big_lifetime = 2000
+
+        config[:config]['timeout'] = big_lifetime
+        expect(subject).to receive(:check_pending_vm).with(vm,pool,big_lifetime,vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should use a pool timeout of 15 if nothing is set' do
+        expect(subject).to receive(:check_pending_vm).with(vm,pool,15,vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+    end
+
+    # COMPLETED
+    context 'Completed VM not in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([new_vm]))
+        expect(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue")
+        expect(logger).to receive(:log).with('s', "[!] [#{pool}] '#{vm}' not found in inventory, removed from 'completed' queue")
+        create_completed_vm(vm,pool,true)
+      end
+
+      it 'should log a message' do
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should not call destroy_vm' do
+        expect(subject).to receive(:destroy_vm).exactly(0).times
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should remove redis information' do
+        expect(redis.sismember("vmpooler__completed__#{pool}",vm)).to be(true)
+        expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to_not be(nil)
+        expect(redis.hget("vmpooler__active__#{pool}",vm)).to_not be(nil)
+
+        subject._check_pool(pool_object,vsphere)
+
+        expect(redis.sismember("vmpooler__completed__#{pool}",vm)).to be(false)
+        expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to be(nil)
+        expect(redis.hget("vmpooler__active__#{pool}",vm)).to be(nil)
+      end
+    end
+
+    context 'Completed VM in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+        create_completed_vm(vm,pool,true)
+      end
+
+      it 'should call destroy_vm' do
+        expect(subject).to receive(:destroy_vm)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      context 'with an error during destroy_vm' do
+        before(:each) do
+          expect(subject).to receive(:destroy_vm).and_raise(RuntimeError,"MockError")
+          expect(logger).to receive(:log).with('d', "[!] [#{pool}] _check_pool failed with an error while evaluating completed VMs: MockError")
+        end
+
+        it 'should log a message' do
+          subject._check_pool(pool_object,vsphere)
+        end
+
+        it 'should remove redis information' do
+          expect(redis.sismember("vmpooler__completed__#{pool}",vm)).to be(true)
+          expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to_not be(nil)
+          expect(redis.hget("vmpooler__active__#{pool}",vm)).to_not be(nil)
+
+          subject._check_pool(pool_object,vsphere)
+
+          expect(redis.sismember("vmpooler__completed__#{pool}",vm)).to be(false)
+          expect(redis.hget("vmpooler__vm__#{vm}", 'checkout')).to be(nil)
+          expect(redis.hget("vmpooler__active__#{pool}",vm)).to be(nil)
+        end
+      end
+    end
+
+    # DISCOVERED
+    context 'Discovered VM' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+        create_discovered_vm(vm,pool)
+      end
+
+      it 'should be moved to the completed queue' do
+        subject._check_pool(pool_object,vsphere)
+
+        expect(redis.sismember("vmpooler__completed__#{pool}", vm)).to be(true)
+      end
+
+      it 'should log a message if an error occurs' do
+        expect(redis).to receive(:smove).with("vmpooler__discovered__#{pool}", "vmpooler__completed__#{pool}", vm).and_raise(RuntimeError,'MockError')
+        expect(logger).to receive(:log).with("d", "[!] [#{pool}] _check_pool failed with an error while evaluating discovered VMs: MockError")
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      ['pending','ready','running','completed'].each do |queue_name|
+        context "exists in the #{queue_name} queue" do
+          before(:each) do
+            allow(subject).to receive(:migrate_vm)
+            allow(subject).to receive(:check_running_vm)
+            allow(subject).to receive(:check_ready_vm)
+            allow(subject).to receive(:check_pending_vm)
+            allow(subject).to receive(:destroy_vm)
+            allow(subject).to receive(:clone_vm)
+          end
+
+          it "should remain in the #{queue_name} queue" do
+            redis.sadd("vmpooler__#{queue_name}__#{pool}", vm)
+            allow(logger).to receive(:log)
+
+            subject._check_pool(pool_object,vsphere)
+
+            expect(redis.sismember("vmpooler__#{queue_name}__#{pool}", vm)).to be(true)
+          end
+
+          it "should be removed from the discovered queue" do
+            redis.sadd("vmpooler__#{queue_name}__#{pool}", vm)
+            allow(logger).to receive(:log)
+
+            expect(redis.sismember("vmpooler__discovered__#{pool}", vm)).to be(true)
+            subject._check_pool(pool_object,vsphere)
+            expect(redis.sismember("vmpooler__discovered__#{pool}", vm)).to be(false)
+          end
+
+          it "should log a message" do
+            redis.sadd("vmpooler__#{queue_name}__#{pool}", vm)
+            expect(logger).to receive(:log).with('d', "[!] [#{pool}] '#{vm}' found in '#{queue_name}', removed from 'discovered' queue")
+
+            subject._check_pool(pool_object,vsphere)
+          end
+        end
+      end
+    end
+
+    # MIGRATIONS
+    context 'Migrating VM not in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([new_vm]))
+        expect(logger).to receive(:log).with('s', "[?] [#{pool}] '#{new_vm}' added to 'discovered' queue")
+        create_migrating_vm(vm,pool)
+      end
+
+      it 'should not do anything' do
+        expect(subject).to receive(:migrate_vm).exactly(0).times
+
+        subject._check_pool(pool_object,vsphere)
+      end
+    end
+
+    context 'Migrating VM in the inventory' do
+      before(:each) do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+        allow(subject).to receive(:check_ready_vm)
+        allow(logger).to receive(:log).with("s", "[!] [#{pool}] is empty")
+        create_migrating_vm(vm,pool)
+      end
+
+      it 'should log an error if one occurs' do
+        expect(subject).to receive(:migrate_vm).and_raise(RuntimeError,'MockError')
+        expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' failed to migrate: MockError")
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should call migrate_vm' do
+        expect(subject).to receive(:migrate_vm).with(vm,pool,vsphere)
+
+        subject._check_pool(pool_object,vsphere)
+      end
+    end
+
+    # REPOPULATE
+    context 'Repopulate a pool' do
+      it 'should not call clone_vm when number of VMs is equal to the pool size' do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([]))
+        expect(subject).to receive(:clone_vm).exactly(0).times
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      it 'should not call clone_vm when number of VMs is greater than the pool size' do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+        create_ready_vm(pool,vm,token)
+        expect(subject).to receive(:clone_vm).exactly(0).times
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      ['ready','pending'].each do |queue_name|
+        it "should use VMs in #{queue_name} queue to caculate pool size" do
+          expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+          expect(subject).to receive(:clone_vm).exactly(0).times
+          # Modify the pool size to 1 and add a VM in the queue
+          redis.sadd("vmpooler__#{queue_name}__#{pool}",vm)
+          config[:pools][0]['size'] = 1
+          
+          subject._check_pool(pool_object,vsphere)
+        end
+      end
+
+      ['running','completed','discovered','migrating'].each do |queue_name|
+        it "should not use VMs in #{queue_name} queue to caculate pool size" do
+          expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([vm]))
+          expect(subject).to receive(:clone_vm)
+          # Modify the pool size to 1 and add a VM in the queue
+          redis.sadd("vmpooler__#{queue_name}__#{pool}",vm)
+          config[:pools][0]['size'] = 1
+
+          subject._check_pool(pool_object,vsphere)
+        end
+      end
+
+      it 'should log a message the first time a pool is empty' do
+        expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([]))
+        expect(logger).to receive(:log).with('s', "[!] [#{pool}] is empty")
+
+        subject._check_pool(pool_object,vsphere)
+      end
+
+      context 'when pool is marked as empty' do
+        before(:each) do
+          expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([]))
+          redis.set("vmpooler__empty__#{pool}", 'true')
+        end
+
+        it 'should not log a message when the pool remains empty' do
+          expect(logger).to receive(:log).with('s', "[!] [#{pool}] is empty").exactly(0).times
+
+          subject._check_pool(pool_object,vsphere)
+        end
+
+        it 'should remove the empty pool mark if it is no longer empty' do
+          create_ready_vm(pool,vm,token)
+
+          expect(redis.get("vmpooler__empty__#{pool}")).to be_truthy
+          subject._check_pool(pool_object,vsphere)
+          expect(redis.get("vmpooler__empty__#{pool}")).to be_falsey
+        end
+      end
+
+      context 'when number of VMs is less than the pool size' do
+        before(:each) do
+          expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([]))
+        end
+
+        it 'should call clone_vm to populate the pool' do
+          pool_size = 5
+          config[:pools][0]['size'] = pool_size
+
+          expect(subject).to receive(:clone_vm).exactly(pool_size).times
+          
+          subject._check_pool(pool_object,vsphere)
+        end
+
+        it 'should call clone_vm until task_limit is hit' do
+          task_limit = 2
+          pool_size = 5
+          config[:pools][0]['size'] = pool_size
+          config[:config]['task_limit'] = task_limit
+
+          expect(subject).to receive(:clone_vm).exactly(task_limit).times
+          
+          subject._check_pool(pool_object,vsphere)
+        end
+
+        it 'log a message if a cloning error occurs' do
+          pool_size = 1
+          config[:pools][0]['size'] = pool_size
+
+          expect(subject).to receive(:clone_vm).and_raise(RuntimeError,"MockError")
+          expect(logger).to receive(:log).with("s", "[!] [#{pool}] clone failed during check_pool with an error: MockError")
+          expect(logger).to receive(:log).with('d', "[!] [#{pool}] _check_pool failed with an error: MockError")
+          
+          expect{ subject._check_pool(pool_object,vsphere) }.to raise_error(RuntimeError,'MockError')
+        end
+      end
+
+      context 'export metrics' do
+        it 'increments metrics for ready queue' do
+          create_ready_vm(pool,'vm1')
+          create_ready_vm(pool,'vm2')
+          create_ready_vm(pool,'vm3')
+          expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new(['vm1','vm2','vm3']))
+
+          expect(metrics).to receive(:gauge).with("ready.#{pool}", 3)
+          allow(metrics).to receive(:gauge)
+
+          subject._check_pool(pool_object,vsphere)
+        end
+
+        it 'increments metrics for running queue' do
+          create_running_vm(pool,'vm1',token)
+          create_running_vm(pool,'vm2',token)
+          create_running_vm(pool,'vm3',token)
+          expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new(['vm1','vm2','vm3']))
+
+          expect(metrics).to receive(:gauge).with("running.#{pool}", 3)
+          allow(metrics).to receive(:gauge)
+
+          subject._check_pool(pool_object,vsphere)
+        end
+
+        it 'increments metrics with 0 when pool empty' do
+          expect(vsphere).to receive(:find_folder).and_return(MockFindFolder.new([]))
+
+          expect(metrics).to receive(:gauge).with("ready.#{pool}", 0)
+          expect(metrics).to receive(:gauge).with("running.#{pool}", 0)
+
+          subject._check_pool(pool_object,vsphere)
+        end
       end
     end
   end

From 943d3025d1aaabd9b79dd6cec99489948ca35290 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 12:02:45 -0800
Subject: [PATCH 20/35] (POOLER-73) Remove #_stats_running_ready tests

This commit removes the #_stats_running_ready tests as they are covered in the
_check_pool tests.
---
 spec/unit/pool_manager_spec.rb | 48 ----------------------------------
 1 file changed, 48 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 12feed2e9..83e73e768 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1761,54 +1761,6 @@
     end
   end
 
-  describe '#_stats_running_ready' do
-    let(:pool_helper) { double('pool') }
-    let(:vsphere) { {pool => pool_helper} }
-    let(:metrics) { Vmpooler::DummyStatsd.new }
-    let(:config) { {
-      config: { task_limit: 10 },
-      pools: [ {'name' => 'pool1', 'size' => 5} ],
-      graphite: { 'prefix' => 'vmpooler' }
-    } }
-
-    before do
-      expect(subject).not_to be_nil
-      $vsphere = vsphere
-      allow(logger).to receive(:log)
-      allow(pool_helper).to receive(:find_folder)
-      allow(redis).to receive(:smembers).and_return([])
-      allow(redis).to receive(:set)
-      allow(redis).to receive(:get).with('vmpooler__tasks__clone').and_return(0)
-      allow(redis).to receive(:get).with('vmpooler__empty__pool1').and_return(nil)
-    end
-
-    context 'metrics' do
-      subject { Vmpooler::PoolManager.new(config, logger, redis, metrics) }
-
-      it 'increments metrics' do
-        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(1)
-        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
-
-        expect(metrics).to receive(:gauge).with('ready.pool1', 1)
-        expect(metrics).to receive(:gauge).with('running.pool1', 5)
-        subject._check_pool(config[:pools][0], vsphere)
-      end
-
-      it 'increments metrics when ready with 0 when pool empty' do
-        allow(redis).to receive(:scard).with('vmpooler__ready__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__cloning__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__pending__pool1').and_return(0)
-        allow(redis).to receive(:scard).with('vmpooler__running__pool1').and_return(5)
-
-        expect(metrics).to receive(:gauge).with('ready.pool1', 0)
-        expect(metrics).to receive(:gauge).with('running.pool1', 5)
-        subject._check_pool(config[:pools][0], vsphere)
-      end
-    end
-  end
-
   describe '#_create_vm_snapshot' do
     let(:snapshot_manager) { 'snapshot_manager' }
     let(:pool_helper) { double('snapshot_manager') }

From 6f79c438c77c12d423317e74754006c5dd7f5bd4 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 20:14:24 -0800
Subject: [PATCH 21/35] (POOLER-73) Add spec tests for create_vm_disk

Add spec tests for create_vm_disk
---
 spec/unit/pool_manager_spec.rb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 83e73e768..cda2ff70d 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -907,6 +907,22 @@
     end
   end
 
+  describe '#create_vm_disk' do
+    let(:vsphere) { double('vsphere') }
+    let(:disk_size) { 15 }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'calls _create_vm_disk' do
+      expect(Thread).to receive(:new).and_yield
+      expect(subject).to receive(:_create_vm_disk).with(vm, disk_size, vsphere)
+
+      subject.create_vm_disk(vm, disk_size, vsphere)
+    end
+  end
+
   describe "#get_vm_host_info" do
     before do
       expect(subject).not_to be_nil

From e6be5bfb75448f7d6ed8728cd838e5e1bfd6b6d2 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 20:17:06 -0800
Subject: [PATCH 22/35] (POOLER-73) Add spec tests for create_vm_snapshot

Add spec tests for create_vm_snapshot
---
 spec/unit/pool_manager_spec.rb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index cda2ff70d..1be23d131 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -923,6 +923,22 @@
     end
   end
 
+  describe '#create_vm_snapshot' do
+    let(:vsphere) { double('vsphere') }
+    let(:snapshot_name) { 'snapshot' }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'calls _create_vm_snapshot' do
+      expect(Thread).to receive(:new).and_yield
+      expect(subject).to receive(:_create_vm_snapshot).with(vm, snapshot_name, vsphere)
+
+      subject.create_vm_snapshot(vm, snapshot_name, vsphere)
+    end
+  end
+
   describe "#get_vm_host_info" do
     before do
       expect(subject).not_to be_nil

From 713e202d680429c7aed46875c32d0a68c0f9a414 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 20:18:08 -0800
Subject: [PATCH 23/35] (POOLER-73) Add spec tests for revert_vm_snapshot

Add spec tests for revert_vm_snapshot
---
 spec/unit/pool_manager_spec.rb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 1be23d131..a3859f00b 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -939,6 +939,22 @@
     end
   end
 
+  describe '#revert_vm_snapshot' do
+    let(:vsphere) { double('vsphere') }
+    let(:snapshot_name) { 'snapshot' }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'calls _create_vm_snapshot' do
+      expect(Thread).to receive(:new).and_yield
+      expect(subject).to receive(:_revert_vm_snapshot).with(vm, snapshot_name, vsphere)
+
+      subject.revert_vm_snapshot(vm, snapshot_name, vsphere)
+    end
+  end
+
   describe "#get_vm_host_info" do
     before do
       expect(subject).not_to be_nil

From 925071be79fef0da279581a6807115d13657bb5c Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 20:19:35 -0800
Subject: [PATCH 24/35] (POOLER-73) Add spec tests for migrate_vm

Add spect tests for migrate_vm
---
 spec/unit/pool_manager_spec.rb | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index a3859f00b..3e2fb76bf 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -955,6 +955,21 @@
     end
   end
 
+  describe '#migrate_vm' do
+    let(:vsphere) { double('vsphere') }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'calls _migrate_vm' do
+      expect(Thread).to receive(:new).and_yield
+      expect(subject).to receive(:_migrate_vm).with(vm, pool, vsphere)
+
+      subject.migrate_vm(vm, pool, vsphere)
+    end
+  end
+
   describe "#get_vm_host_info" do
     before do
       expect(subject).not_to be_nil

From a4c55d5bf4928a434ade8af8cb3e16a72df501cd Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Mon, 13 Feb 2017 20:28:04 -0800
Subject: [PATCH 25/35] (POOLER-73) Add spec tests for migration_limit

Add spec tests for migration_limit
---
 spec/unit/pool_manager_spec.rb | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 3e2fb76bf..2b77f0c53 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -955,6 +955,28 @@
     end
   end
 
+  describe '#migration_limit' do
+    # This is a little confusing.  Is this supposed to return a boolean
+    # or integer type?
+    [false,0].each do |testvalue|
+      it "should return false for an input of #{testvalue}" do
+        expect(subject.migration_limit(testvalue)).to eq(false)
+      end
+    end
+
+    [1,32768].each do |testvalue|
+      it "should return #{testvalue} for an input of #{testvalue}" do
+        expect(subject.migration_limit(testvalue)).to eq(testvalue)
+      end
+    end
+
+    [-1,-32768].each do |testvalue|
+      it "should return nil for an input of #{testvalue}" do
+        expect(subject.migration_limit(testvalue)).to be_nil
+      end
+    end
+  end
+
   describe '#migrate_vm' do
     let(:vsphere) { double('vsphere') }
 

From fb6be8e0792703e5695572108119b1d67c97dfc2 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Tue, 14 Feb 2017 08:57:44 -0800
Subject: [PATCH 26/35] (POOLER-73) Add spec tests for
 remove_vmpooler_migration_vm

Add spec tests for remove_vmpooler_migration_vm
---
 spec/unit/pool_manager_spec.rb | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 2b77f0c53..08b14ea10 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1302,6 +1302,25 @@
     end
   end
 
+  describe '#remove_vmpooler_migration_vm' do
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    it 'should remove the migration from redis' do
+      redis.sadd('vmpooler__migration', vm)
+      expect(redis.sismember('vmpooler__migration',vm)).to be(true)
+      subject.remove_vmpooler_migration_vm(pool, vm)
+      expect(redis.sismember('vmpooler__migration',vm)).to be(false)
+    end
+
+    it 'should log a message and swallow an error if one occurs' do
+      expect(redis).to receive(:srem).and_raise(RuntimeError,'MockError')
+      expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' removal from vmpooler__migration failed with an error: MockError")
+      subject.remove_vmpooler_migration_vm(pool, vm)
+    end
+  end
+
   describe '#_check_pool' do
     # Default test fixtures will consist of;
     #   - Empty Redis dataset

From 47d597f68ab725c294e0fac215bd5f04f9f83c18 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Tue, 14 Feb 2017 09:15:54 -0800
Subject: [PATCH 27/35] (POOLER-73) Add spec tests for
 migrate_vm_and_record_timing

Add spec tests for migrate_vm_and_record_timing
---
 spec/unit/pool_manager_spec.rb | 44 ++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 08b14ea10..375a3554b 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1321,6 +1321,50 @@
     end
   end
 
+  describe '#migrate_vm_and_record_timing' do
+    let(:vsphere) { double('vsphere') }
+    let(:vm_object) { double('vm_object') }
+    let(:source_host_name) { 'source_host' }
+    let(:dest_host_name) { 'dest_host' }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      create_vm(vm,token)
+      expect(vsphere).to receive(:migrate_vm_host).with(vm_object, host)
+    end
+
+    it 'should return the elapsed time for the migration' do
+      result = subject.migrate_vm_and_record_timing(vm_object, vm, pool, host, source_host_name, dest_host_name, vsphere)
+      expect(result).to match(/0\.[\d]+/)
+    end
+
+    it 'should add timing metric' do
+      expect(metrics).to receive(:timing).with("migrate.#{pool}",String)
+      subject.migrate_vm_and_record_timing(vm_object, vm, pool, host, source_host_name, dest_host_name, vsphere)
+    end
+
+    it 'should increment from_host and to_host metric' do
+      expect(metrics).to receive(:increment).with("migrate_from.#{source_host_name}")
+      expect(metrics).to receive(:increment).with("migrate_to.#{dest_host_name}")
+      subject.migrate_vm_and_record_timing(vm_object, vm, pool, host, source_host_name, dest_host_name, vsphere)
+    end
+
+    it 'should set migration_time metric in redis' do
+      expect(redis.hget("vmpooler__vm__#{vm}", 'migration_time')).to be_nil
+      subject.migrate_vm_and_record_timing(vm_object, vm, pool, host, source_host_name, dest_host_name, vsphere)
+      expect(redis.hget("vmpooler__vm__#{vm}", 'migration_time')).to match(/0\.[\d]+/)
+    end
+
+    it 'should set checkout_to_migration metric in redis' do
+      expect(redis.hget("vmpooler__vm__#{vm}", 'checkout_to_migration')).to be_nil
+      subject.migrate_vm_and_record_timing(vm_object, vm, pool, host, source_host_name, dest_host_name, vsphere)
+      expect(redis.hget("vmpooler__vm__#{vm}", 'checkout_to_migration')).to match(/0\.[\d]+/)
+    end
+  end
+
   describe '#_check_pool' do
     # Default test fixtures will consist of;
     #   - Empty Redis dataset

From 4dd0c96a78a1848f32dff68778a625c7e34ba915 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 17 Feb 2017 11:17:17 -0800
Subject: [PATCH 28/35] (POOLER-73) Add spec tests for check_disk_queue

Add spec tests for check_disk_queue

Previously the check_disk_queue method would execute the loop indefinitely as it
did not have a terminating condition.  This made it impossible to test.  This
commit modifies the check_disk_queue method so that it can take a maxloop and
delay parameter so that it can be tested.
---
 lib/vmpooler/pool_manager.rb   | 11 ++++--
 spec/unit/pool_manager_spec.rb | 66 ++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index 2892adc89..aa89fc14f 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -412,15 +412,20 @@ def _revert_vm_snapshot(vm, snapshot_name, vsphere)
       end
     end
 
-    def check_disk_queue
+    def check_disk_queue(maxloop = 0, loop_delay = 5)
       $logger.log('d', "[*] [disk_manager] starting worker thread")
 
       $vsphere['disk_manager'] ||= Vmpooler::VsphereHelper.new $config, $metrics
-
       $threads['disk_manager'] = Thread.new do
+        loop_count = 1
         loop do
           _check_disk_queue $vsphere['disk_manager']
-          sleep(5)
+          sleep(loop_delay)
+
+          unless maxloop.zero?
+            break if loop_count >= maxloop
+            loop_count = loop_count + 1
+          end
         end
       end
     end
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 375a3554b..36cd9b3cf 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -955,6 +955,72 @@
     end
   end
 
+  describe '#check_disk_queue' do
+    let(:threads) {[]}
+
+    before(:each) do
+      expect(Thread).to receive(:new).and_yield
+      allow(subject).to receive(:_check_disk_queue)
+    end
+
+    it 'should log the disk manager is starting' do
+      expect(logger).to receive(:log).with('d', "[*] [disk_manager] starting worker thread")
+
+      expect($threads.count).to be(0)
+      subject.check_disk_queue(1,0)
+      expect($threads.count).to be(1)
+    end
+
+    it 'should add the manager to the global thread list' do
+      # Note - Ruby core types are not necessarily thread safe
+      expect($threads.count).to be(0)
+      subject.check_disk_queue(1,0)
+      expect($threads.count).to be(1)
+    end
+
+    it 'should call _check_disk_queue' do
+      expect(subject).to receive(:_check_disk_queue).with(Vmpooler::VsphereHelper)
+
+      subject.check_disk_queue(1,0)
+    end
+
+    context 'delays between loops' do
+      let(:maxloop) { 2 }
+      let(:loop_delay) { 1 }
+      # Note a maxloop of zero can not be tested as it never terminates
+
+      it 'defaults to 5 second loop delay' do
+        expect(subject).to receive(:sleep).with(5).exactly(maxloop).times
+        subject.check_disk_queue(maxloop)
+      end
+
+      it 'when a non-default loop delay is specified' do
+        start_time = Time.now
+        subject.check_disk_queue(maxloop,loop_delay)
+        finish_time = Time.now
+
+        # Use a generous delta to take into account various CPU load etc.
+        expect(finish_time - start_time).to be_within(0.75).of(maxloop * loop_delay)
+      end
+    end
+
+    context 'loops specified number of times (5)' do
+      let(:maxloop) { 5 }
+      # Note a maxloop of zero can not be tested as it never terminates
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+      end
+
+      it 'should call _check_disk_queue 5 times' do
+        expect(subject).to receive(:_check_disk_queue).with(Vmpooler::VsphereHelper).exactly(maxloop).times
+
+        subject.check_disk_queue(maxloop,0)
+      end
+    end
+  end
+
   describe '#migration_limit' do
     # This is a little confusing.  Is this supposed to return a boolean
     # or integer type?

From 6f127d32bcdfbc122b0c4e0493f54ed376251515 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 17 Feb 2017 11:21:46 -0800
Subject: [PATCH 29/35] (POOLER-73) Add spec tests for check_snapshot_queue

Add spec tests for check_snapshot_queue

Previously the check_snapshot_queue method would execute the loop indefinitely
as it did not have a terminating condition.  This made it impossible to test.
This commit modifies the check_snapshot_queue method so that it can take a
maxloop and delay parameter so that it can be tested.
---
 lib/vmpooler/pool_manager.rb   | 10 ++++--
 spec/unit/pool_manager_spec.rb | 66 ++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/lib/vmpooler/pool_manager.rb b/lib/vmpooler/pool_manager.rb
index aa89fc14f..f9103efe8 100644
--- a/lib/vmpooler/pool_manager.rb
+++ b/lib/vmpooler/pool_manager.rb
@@ -443,15 +443,21 @@ def _check_disk_queue(vsphere)
       end
     end
 
-    def check_snapshot_queue
+    def check_snapshot_queue(maxloop = 0, loop_delay = 5)
       $logger.log('d', "[*] [snapshot_manager] starting worker thread")
 
       $vsphere['snapshot_manager'] ||= Vmpooler::VsphereHelper.new $config, $metrics
 
       $threads['snapshot_manager'] = Thread.new do
+        loop_count = 1
         loop do
           _check_snapshot_queue $vsphere['snapshot_manager']
-          sleep(5)
+          sleep(loop_delay)
+
+          unless maxloop.zero?
+            break if loop_count >= maxloop
+            loop_count = loop_count + 1
+          end
         end
       end
     end
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 36cd9b3cf..5d1e2570c 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1021,6 +1021,72 @@
     end
   end
 
+  describe '#check_snapshot_queue' do
+    let(:threads) {[]}
+
+    before(:each) do
+      expect(Thread).to receive(:new).and_yield
+      allow(subject).to receive(:_check_snapshot_queue)
+    end
+
+    it 'should log the disk manager is starting' do
+      expect(logger).to receive(:log).with('d', "[*] [snapshot_manager] starting worker thread")
+
+      expect($threads.count).to be(0)
+      subject.check_snapshot_queue(1,0)
+      expect($threads.count).to be(1)
+    end
+
+    it 'should add the manager to the global thread list' do
+      # Note - Ruby core types are not necessarily thread safe
+      expect($threads.count).to be(0)
+      subject.check_snapshot_queue(1,0)
+      expect($threads.count).to be(1)
+    end
+
+    it 'should call _check_snapshot_queue' do
+      expect(subject).to receive(:_check_snapshot_queue).with(Vmpooler::VsphereHelper)
+
+      subject.check_snapshot_queue(1,0)
+    end
+
+    context 'delays between loops' do
+      let(:maxloop) { 2 }
+      let(:loop_delay) { 1 }
+      # Note a maxloop of zero can not be tested as it never terminates
+
+      it 'defaults to 5 second loop delay' do
+        expect(subject).to receive(:sleep).with(5).exactly(maxloop).times
+        subject.check_snapshot_queue(maxloop)
+      end
+
+      it 'when a non-default loop delay is specified' do
+        start_time = Time.now
+        subject.check_snapshot_queue(maxloop,loop_delay)
+        finish_time = Time.now
+
+        # Use a generous delta to take into account various CPU load etc.
+        expect(finish_time - start_time).to be_within(0.75).of(maxloop * loop_delay)
+      end
+    end
+
+    context 'loops specified number of times (5)' do
+      let(:maxloop) { 5 }
+      # Note a maxloop of zero can not be tested as it never terminates
+
+      after(:each) do
+        # Reset the global variable - Note this is a code smell
+        $threads = nil
+      end
+
+      it 'should call _check_snapshot_queue 5 times' do
+        expect(subject).to receive(:_check_snapshot_queue).with(Vmpooler::VsphereHelper).exactly(maxloop).times
+
+        subject.check_snapshot_queue(maxloop,0)
+      end
+    end
+  end
+
   describe '#migration_limit' do
     # This is a little confusing.  Is this supposed to return a boolean
     # or integer type?

From 25ad5d58f79eff510ed84dbc16ea148470433071 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 17 Feb 2017 12:57:08 -0800
Subject: [PATCH 30/35] (POOLER-73) Add spec tests for _check_snapshot_queue

Add spec tests for _check_snapshot_queue
---
 spec/unit/pool_manager_spec.rb | 75 ++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 5d1e2570c..dcbfac2eb 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1087,6 +1087,81 @@
     end
   end
 
+  describe '#_check_snapshot_queue' do
+    let(:vsphere) { double('vsphere') }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    context 'vmpooler__tasks__snapshot queue' do
+      context 'when no VMs in the queue' do
+        it 'should not call create_vm_snapshot' do
+          expect(subject).to receive(:create_vm_snapshot).exactly(0).times
+          subject._check_snapshot_queue(vsphere)
+        end
+      end
+
+      context 'when multiple VMs in the queue' do
+        before(:each) do
+          snapshot_vm('vm1','snapshot1')
+          snapshot_vm('vm2','snapshot2')
+          snapshot_vm('vm3','snapshot3')
+        end
+
+        it 'should call create_vm_snapshot once' do
+          expect(subject).to receive(:create_vm_snapshot).exactly(1).times
+          subject._check_snapshot_queue(vsphere)
+        end
+
+        it 'should snapshot the first VM in the queue' do
+          expect(subject).to receive(:create_vm_snapshot).with('vm1','snapshot1',vsphere)
+          subject._check_snapshot_queue(vsphere)
+        end
+
+        it 'should log an error if one occurs' do
+          expect(subject).to receive(:create_vm_snapshot).and_raise(RuntimeError,'MockError')
+          expect(logger).to receive(:log).with('s', "[!] [snapshot_manager] snapshot appears to have failed")
+          subject._check_snapshot_queue(vsphere)
+        end
+      end
+    end
+
+    context 'revert_vm_snapshot queue' do
+      context 'when no VMs in the queue' do
+        it 'should not call revert_vm_snapshot' do
+          expect(subject).to receive(:revert_vm_snapshot).exactly(0).times
+          subject._check_snapshot_queue(vsphere)
+        end
+      end
+
+      context 'when multiple VMs in the queue' do
+        before(:each) do
+          snapshot_revert_vm('vm1','snapshot1')
+          snapshot_revert_vm('vm2','snapshot2')
+          snapshot_revert_vm('vm3','snapshot3')
+        end
+
+        it 'should call revert_vm_snapshot once' do
+          expect(subject).to receive(:revert_vm_snapshot).exactly(1).times
+          subject._check_snapshot_queue(vsphere)
+        end
+
+        it 'should revert snapshot the first VM in the queue' do
+          expect(subject).to receive(:revert_vm_snapshot).with('vm1','snapshot1',vsphere)
+          subject._check_snapshot_queue(vsphere)
+        end
+
+        it 'should log an error if one occurs' do
+          expect(subject).to receive(:revert_vm_snapshot).and_raise(RuntimeError,'MockError')
+          expect(logger).to receive(:log).with('s', "[!] [snapshot_manager] snapshot revert appears to have failed")
+          subject._check_snapshot_queue(vsphere)
+        end
+      end
+    end
+  end
+
+
   describe '#migration_limit' do
     # This is a little confusing.  Is this supposed to return a boolean
     # or integer type?

From 3083186241a198911600269bf8c73526f8f03be7 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 17 Feb 2017 13:41:24 -0800
Subject: [PATCH 31/35] (POOLER-73) Add spec tests for _check_disk_queue

Add spec tests for _check_disk_queue
---
 spec/unit/pool_manager_spec.rb | 39 ++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index dcbfac2eb..c8df3f101 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1021,6 +1021,45 @@
     end
   end
 
+  describe '#_check_disk_queue' do
+    let(:vsphere) { double('vsphere') }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    context 'when no VMs in the queue' do
+      it 'should not call create_vm_disk' do
+        expect(subject).to receive(:create_vm_disk).exactly(0).times
+        subject._check_disk_queue(vsphere)
+      end
+    end
+
+    context 'when multiple VMs in the queue' do
+      before(:each) do
+        disk_task_vm('vm1',1)
+        disk_task_vm('vm2',2)
+        disk_task_vm('vm3',3)
+      end
+
+      it 'should call create_vm_disk once' do
+        expect(subject).to receive(:create_vm_disk).exactly(1).times
+        subject._check_disk_queue(vsphere)
+      end
+
+      it 'should snapshot the first VM in the queue' do
+        expect(subject).to receive(:create_vm_disk).with('vm1','1',vsphere)
+        subject._check_disk_queue(vsphere)
+      end
+
+      it 'should log an error if one occurs' do
+        expect(subject).to receive(:create_vm_disk).and_raise(RuntimeError,'MockError')
+        expect(logger).to receive(:log).with('s', "[!] [disk_manager] disk creation appears to have failed")
+        subject._check_disk_queue(vsphere)
+      end
+    end
+  end
+
   describe '#check_snapshot_queue' do
     let(:threads) {[]}
 

From 0a6dffbd05914260d5cff67048bf9c7b87fc529a Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 17 Feb 2017 14:16:13 -0800
Subject: [PATCH 32/35] (POOLER-73) Add spec tests for _create_vm_disk

Add spec tests for _create_vm_disk
---
 spec/unit/pool_manager_spec.rb | 95 ++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index c8df3f101..b3cc53a69 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -923,6 +923,101 @@
     end
   end
 
+  describe "#_create_vm_disk" do
+    let(:vsphere) { double('vsphere') }
+    let(:disk_size) { '15' }
+    let(:datastore) { 'datastore0'}
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:pools:
+  - name: #{pool}
+    datastore: '#{datastore}'
+EOT
+      )
+    }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      allow(vsphere).to receive(:find_vm).with(vm).and_return(host)
+      create_running_vm(pool,vm,token)
+    end
+
+    it 'should not do anything if the VM does not exist' do
+      expect(vsphere).to receive(:find_vm).with(vm).and_return(nil)
+      expect(logger).to receive(:log).exactly(0).times
+      subject._create_vm_disk(vm, disk_size, vsphere)
+    end
+
+    it 'should not do anything if the disk size is nil' do
+      expect(logger).to receive(:log).exactly(0).times
+      subject._create_vm_disk(vm, nil, vsphere)
+    end
+
+    it 'should not do anything if the disk size is empty string' do
+      expect(logger).to receive(:log).exactly(0).times
+      subject._create_vm_disk(vm, '', vsphere)
+    end
+
+    it 'should not do anything if the disk size is less than 1' do
+      expect(logger).to receive(:log).exactly(0).times
+      subject._create_vm_disk(vm, '0', vsphere)
+    end
+
+    it 'should not do anything if the disk size cannot be converted to an integer' do
+      expect(logger).to receive(:log).exactly(0).times
+      subject._create_vm_disk(vm, 'abc123', vsphere)
+    end
+
+    it 'should raise an error if the disk size is a Fixnum' do
+      expect(logger).to receive(:log).exactly(0).times
+      expect{ subject._create_vm_disk(vm, 10, vsphere) }.to raise_error(NoMethodError,/empty?/)
+    end
+
+    it 'should not do anything if the datastore for pool is nil' do
+      expect(logger).to receive(:log).with('s', "[ ] [disk_manager] '#{vm}' is attaching a #{disk_size}gb disk")
+      expect(logger).to receive(:log).with('s', "[+] [disk_manager] '#{vm}' failed to attach disk")
+      config[:pools][0]['datastore'] = nil
+
+      subject._create_vm_disk(vm, disk_size, vsphere)
+    end
+
+    it 'should not do anything if the datastore for pool is empty' do
+      expect(logger).to receive(:log).with('s', "[ ] [disk_manager] '#{vm}' is attaching a #{disk_size}gb disk")
+      expect(logger).to receive(:log).with('s', "[+] [disk_manager] '#{vm}' failed to attach disk")
+      config[:pools][0]['datastore'] = ''
+
+      subject._create_vm_disk(vm, disk_size, vsphere)
+    end
+
+    it 'should attach the disk' do
+      expect(logger).to receive(:log).with('s', "[ ] [disk_manager] '#{vm}' is attaching a #{disk_size}gb disk")
+      expect(logger).to receive(:log).with('s', /\[\+\] \[disk_manager\] '#{vm}' attached #{disk_size}gb disk in 0.[\d]+ seconds/)
+      expect(vsphere).to receive(:add_disk).with(host,disk_size,datastore)
+
+      subject._create_vm_disk(vm, disk_size, vsphere)
+    end
+
+    it 'should update redis information when attaching the first disk' do
+      expect(vsphere).to receive(:add_disk).with(host,disk_size,datastore)
+
+      subject._create_vm_disk(vm, disk_size, vsphere)
+      expect(redis.hget("vmpooler__vm__#{vm}", 'disk')).to eq("+#{disk_size}gb")
+    end
+
+    it 'should update redis information when attaching the additional disks' do
+      expect(vsphere).to receive(:add_disk).with(host,disk_size,datastore)
+      initial_disks = '+10gb:+20gb'
+      redis.hset("vmpooler__vm__#{vm}", 'disk', initial_disks)
+
+      subject._create_vm_disk(vm, disk_size, vsphere)
+      expect(redis.hget("vmpooler__vm__#{vm}", 'disk')).to eq("#{initial_disks}:+#{disk_size}gb")
+    end
+  end
+
   describe '#create_vm_snapshot' do
     let(:vsphere) { double('vsphere') }
     let(:snapshot_name) { 'snapshot' }

From b63822fa9f4cfde51c05b9ce65cbfb1b10a42e99 Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 17 Feb 2017 14:34:20 -0800
Subject: [PATCH 33/35] (POOLER-73) Modify spec tests for _create_vm_snapshot

Modify spec tests for _create_vm_snapshot
---
 spec/unit/pool_manager_spec.rb | 70 ++++++++++++++++++++++------------
 1 file changed, 45 insertions(+), 25 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index b3cc53a69..5c3fc9105 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1034,6 +1034,51 @@
     end
   end
 
+  describe '#_create_vm_snapshot' do
+    let(:vsphere) { double('vsphere') }
+    let(:snapshot_name) { 'snapshot1' }
+    let(:snapshot_task) { double('snapshot_task') }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      allow(vsphere).to receive(:find_vm).with(vm).and_return(host)
+      allow(snapshot_task).to receive(:wait_for_completion).and_return(nil)
+      allow(host).to receive(:CreateSnapshot_Task).with({:name=>snapshot_name, :description=>"vmpooler", :memory=>true, :quiesce=>true}).and_return(snapshot_task)
+      create_running_vm(pool,vm,token)
+    end
+
+    it 'should not do anything if the VM does not exist' do
+      expect(vsphere).to receive(:find_vm).with(vm).and_return(nil)
+      expect(logger).to receive(:log).exactly(0).times
+      subject._create_vm_snapshot(vm, snapshot_name, vsphere)
+    end
+
+    it 'should not do anything if the snapshot name is nil' do
+      expect(logger).to receive(:log).exactly(0).times
+      subject._create_vm_snapshot(vm, nil, vsphere)
+    end
+
+    it 'should not do anything if the snapshot name is empty string' do
+      expect(logger).to receive(:log).exactly(0).times
+      subject._create_vm_snapshot(vm, '', vsphere)
+    end
+
+    it 'should invoke vSphere to snapshot the VM' do
+      expect(logger).to receive(:log).with('s', "[ ] [snapshot_manager] '#{vm}' is being snapshotted")
+      expect(logger).to receive(:log).with('s', /\[\+\] \[snapshot_manager\] '#{vm}' snapshot created in 0.[\d]+ seconds/)
+      subject._create_vm_snapshot(vm, snapshot_name, vsphere)
+    end
+
+    it 'should add snapshot redis information' do
+      expect(redis.hget("vmpooler__vm__#{vm}", "snapshot:#{snapshot_name}")).to be_nil
+      subject._create_vm_snapshot(vm, snapshot_name, vsphere)
+      expect(redis.hget("vmpooler__vm__#{vm}", "snapshot:#{snapshot_name}")).to_not be_nil
+    end
+  end
+
   describe '#revert_vm_snapshot' do
     let(:vsphere) { double('vsphere') }
     let(:snapshot_name) { 'snapshot' }
@@ -2250,31 +2295,6 @@
     end
   end
 
-  describe '#_create_vm_snapshot' do
-    let(:snapshot_manager) { 'snapshot_manager' }
-    let(:pool_helper) { double('snapshot_manager') }
-    let(:vsphere) { {snapshot_manager => pool_helper} }
-
-    before do
-      expect(subject).not_to be_nil
-      $vsphere = vsphere
-    end
-
-    context '(valid host)' do
-      let(:vm_host) { double('vmhost') }
-
-      it 'creates a snapshot' do
-        expect(vsphere).to receive(:find_vm).and_return vm_host
-        expect(logger).to receive(:log)
-        expect(vm_host).to receive_message_chain(:CreateSnapshot_Task, :wait_for_completion)
-        expect(redis).to receive(:hset).with('vmpooler__vm__testvm', 'snapshot:testsnapshot', Time.now.to_s)
-        expect(logger).to receive(:log)
-
-        subject._create_vm_snapshot('testvm', 'testsnapshot', vsphere)
-      end
-    end
-  end
-
   describe '#_revert_vm_snapshot' do
     let(:snapshot_manager) { 'snapshot_manager' }
     let(:pool_helper) { double('snapshot_manager') }

From c69d61107fa476d37c9d68469de044fa63ae9c1e Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 17 Feb 2017 14:44:12 -0800
Subject: [PATCH 34/35] (POOLER-73) Modify spec tests for _revert_vm_snapshot

Modify spec tests for _revert_vm_snapshot
---
 spec/unit/pool_manager_spec.rb | 66 ++++++++++++++++++++--------------
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index 5c3fc9105..ba147abcc 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1095,6 +1095,46 @@
     end
   end
 
+  describe '#_revert_vm_snapshot' do
+    let(:vsphere) { double('vsphere') }
+    let(:snapshot_name) { 'snapshot1' }
+    let(:snapshot_object) { double('snapshot_object') }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    before(:each) do
+      allow(vsphere).to receive(:find_vm).with(vm).and_return(host)
+      allow(snapshot_object).to receive_message_chain(:RevertToSnapshot_Task, :wait_for_completion)
+      allow(vsphere).to receive(:find_snapshot).with(host,snapshot_name).and_return(snapshot_object)
+    end
+
+    it 'should not do anything if the VM does not exist' do
+      expect(vsphere).to receive(:find_vm).with(vm).and_return(nil)
+      expect(logger).to receive(:log).exactly(0).times
+      subject._revert_vm_snapshot(vm, snapshot_name, vsphere)
+    end
+
+    it 'should not do anything if the snapshot name is nil' do
+      expect(logger).to receive(:log).exactly(0).times
+      expect(vsphere).to receive(:find_snapshot).with(host,nil).and_return nil
+      subject._revert_vm_snapshot(vm, nil, vsphere)
+    end
+
+    it 'should not do anything if the snapshot name is empty string' do
+      expect(logger).to receive(:log).exactly(0).times
+      expect(vsphere).to receive(:find_snapshot).with(host,'').and_return nil
+      subject._revert_vm_snapshot(vm, '', vsphere)
+    end
+
+    it 'should invoke vSphere to revert the VM to the snapshot' do
+      expect(logger).to receive(:log).with('s', "[ ] [snapshot_manager] '#{vm}' is being reverted to snapshot '#{snapshot_name}'")
+      expect(logger).to receive(:log).with('s', /\[\<\] \[snapshot_manager\] '#{vm}' reverted to snapshot in 0\.[\d]+ seconds/)
+      subject._revert_vm_snapshot(vm, snapshot_name, vsphere)
+    end
+  end
+
   describe '#check_disk_queue' do
     let(:threads) {[]}
 
@@ -2295,32 +2335,6 @@
     end
   end
 
-  describe '#_revert_vm_snapshot' do
-    let(:snapshot_manager) { 'snapshot_manager' }
-    let(:pool_helper) { double('snapshot_manager') }
-    let(:vsphere) { {snapshot_manager => pool_helper} }
-
-    before do
-      expect(subject).not_to be_nil
-      $vsphere = vsphere
-    end
-
-    context '(valid host)' do
-      let(:vm_host) { double('vmhost') }
-      let(:vm_snapshot) { double('vmsnapshot') }
-
-      it 'reverts a snapshot' do
-        expect(vsphere).to receive(:find_vm).and_return vm_host
-        expect(vsphere).to receive(:find_snapshot).and_return vm_snapshot
-        expect(logger).to receive(:log)
-        expect(vm_snapshot).to receive_message_chain(:RevertToSnapshot_Task, :wait_for_completion)
-        expect(logger).to receive(:log)
-
-        subject._revert_vm_snapshot('testvm', 'testsnapshot', vsphere)
-      end
-    end
-  end
-
   describe '#_check_snapshot_queue' do
     let(:pool_helper) { double('pool') }
     let(:vsphere) { {pool => pool_helper} }

From e783e937840006a737c580def51d851448a5997d Mon Sep 17 00:00:00 2001
From: Glenn Sarti <glennsarti@users.noreply.github.com>
Date: Fri, 17 Feb 2017 15:46:35 -0800
Subject: [PATCH 35/35] (POOLER-73) Modify spec tests for _migrate_vm

Modify spec tests for _migrate_vm
---
 spec/unit/pool_manager_migration_spec.rb |  87 -------------
 spec/unit/pool_manager_spec.rb           | 156 +++++++++++++++++++++++
 2 files changed, 156 insertions(+), 87 deletions(-)
 delete mode 100644 spec/unit/pool_manager_migration_spec.rb

diff --git a/spec/unit/pool_manager_migration_spec.rb b/spec/unit/pool_manager_migration_spec.rb
deleted file mode 100644
index 9fd491b01..000000000
--- a/spec/unit/pool_manager_migration_spec.rb
+++ /dev/null
@@ -1,87 +0,0 @@
-require 'spec_helper'
-require 'mock_redis'
-require 'time'
-
-describe 'Pool Manager' do
-  let(:logger) { double('logger') }
-  let(:redis) { MockRedis.new }
-  let(:metrics) { Vmpooler::DummyStatsd.new }
-  let(:config) {
-    {
-      config: {
-        'site_name' => 'test pooler',
-        'migration_limit' => 2,
-        vsphere: {
-          'server' => 'vsphere.puppet.com',
-          'username' => 'vmpooler@vsphere.local',
-          'password' => '',
-          'insecure' => true
-        },
-        pools: [ {'name' => 'pool1', 'size' => 5, 'folder' => 'pool1_folder'} ],
-        statsd: { 'prefix' => 'stats_prefix'},
-        pool_names: [ 'pool1' ]
-      }
-    }
-  }
-  let(:pool) { config[:config][:pools][0]['name'] }
-  let(:vm) {
-    {
-      'name' => 'vm1',
-      'host' => 'host1',
-      'template' => pool,
-    }
-  }
-
-  describe '#_migrate_vm' do
-    let(:vsphere) { double(pool) }
-    let(:pooler) { Vmpooler::PoolManager.new(config, logger, redis, metrics) }
-    context 'evaluates VM for migration and logs host' do
-      before do
-        create_migrating_vm vm['name'], pool, redis
-        allow(vsphere).to receive(:find_vm).and_return(vm)
-        allow(pooler).to receive(:get_vm_host_info).and_return([{'name' => 'host1'}, 'host1'])
-      end
-
-      it 'logs VM host when migration is disabled' do
-        config[:config]['migration_limit'] = nil
-
-        expect(redis.sismember("vmpooler__migrating__#{pool}", vm['name'])).to be true
-        expect(logger).to receive(:log).with('s', "[ ] [#{pool}] '#{vm['name']}' is running on #{vm['host']}")
-
-        pooler._migrate_vm(vm['name'], pool, vsphere)
-
-        expect(redis.sismember("vmpooler__migrating__#{pool}", vm['name'])).to be false
-      end
-
-      it 'verifies that migration_limit greater than or equal to migrations in progress and logs host' do
-        add_vm_to_migration_set vm['name'], redis
-        add_vm_to_migration_set 'vm2', redis
-
-        expect(logger).to receive(:log).with('s', "[ ] [#{pool}] '#{vm['name']}' is running on #{vm['host']}. No migration will be evaluated since the migration_limit has been reached")
-
-        pooler._migrate_vm(vm['name'], pool, vsphere)
-      end
-
-      it 'verifies that migration_limit is less than migrations in progress and logs old host, new host and migration time' do
-        allow(vsphere).to receive(:find_least_used_compatible_host).and_return([{'name' => 'host2'}, 'host2'])
-        allow(vsphere).to receive(:migrate_vm_host)
-
-        expect(redis.hget("vmpooler__vm__#{vm['name']}", 'migration_time'))
-        expect(redis.hget("vmpooler__vm__#{vm['name']}", 'checkout_to_migration'))
-        expect(logger).to receive(:log).with('s', "[>] [#{pool}] '#{vm['name']}' migrated from #{vm['host']} to host2 in 0.00 seconds")
-
-        pooler._migrate_vm(vm['name'], pool, vsphere)
-      end
-
-      it 'fails when no suitable host can be found' do
-        error = 'ArgumentError: No target host found'
-        allow(vsphere).to receive(:find_least_used_compatible_host)
-        allow(vsphere).to receive(:migrate_vm_host).and_raise(error)
-
-        expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm['name']}' migration failed with an error: #{error}")
-
-        pooler._migrate_vm(vm['name'], pool, vsphere)
-      end
-    end
-  end
-end
diff --git a/spec/unit/pool_manager_spec.rb b/spec/unit/pool_manager_spec.rb
index ba147abcc..825d08294 100644
--- a/spec/unit/pool_manager_spec.rb
+++ b/spec/unit/pool_manager_spec.rb
@@ -1418,6 +1418,162 @@
     end
   end
 
+  describe "#_migrate_vm" do
+    let(:vsphere) { double('vsphere') }
+    let(:vm_parent_hostname) { 'parent1' }
+    let(:config) {
+      YAML.load(<<-EOT
+---
+:config:
+  migration_limit: 5
+:pools:
+  - name: #{pool}
+EOT
+      )
+    }
+
+    before do
+      expect(subject).not_to be_nil
+    end
+
+    context 'when an error occurs' do
+      it 'should log an error message and attempt to remove from vmpooler_migration queue' do
+        expect(vsphere).to receive(:find_vm).with(vm).and_raise(RuntimeError,'MockError')
+        expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' migration failed with an error: MockError")
+        expect(subject).to receive(:remove_vmpooler_migration_vm)
+        subject._migrate_vm(vm, pool, vsphere)
+      end
+    end
+
+    context 'when VM does not exist' do
+      it 'should log an error message when VM does not exist' do
+        expect(vsphere).to receive(:find_vm).with(vm).and_return(nil)
+        # This test is quite fragile.  Should refactor the code to make this scenario easier to detect
+        expect(logger).to receive(:log).with('s', "[x] [#{pool}] '#{vm}' migration failed with an error: undefined method `summary' for nil:NilClass")
+        subject._migrate_vm(vm, pool, vsphere)
+      end
+    end
+
+    context 'when VM exists but migration is disabled' do
+      before(:each) do
+        expect(vsphere).to receive(:find_vm).with(vm).and_return(host)
+        allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname])
+        create_migrating_vm(vm, pool)
+      end
+
+      [-1,-32768,false,0].each do |testvalue|
+        it "should not migrate a VM if the migration limit is #{testvalue}" do
+          config[:config]['migration_limit'] = testvalue
+          expect(logger).to receive(:log).with('s', "[ ] [#{pool}] '#{vm}' is running on #{vm_parent_hostname}")
+          subject._migrate_vm(vm, pool, vsphere)
+        end
+
+        it "should remove the VM from vmpooler__migrating queue in redis if the migration limit is #{testvalue}" do
+          redis.sadd("vmpooler__migrating__#{pool}", vm)
+          config[:config]['migration_limit'] = testvalue
+
+          expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_truthy
+          subject._migrate_vm(vm, pool, vsphere)
+          expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_falsey
+        end
+      end
+    end
+
+    context 'when VM exists but migration limit is reached' do
+      before(:each) do
+        expect(vsphere).to receive(:find_vm).with(vm).and_return(host)
+        allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname])
+
+        create_migrating_vm(vm, pool)
+        redis.sadd('vmpooler__migration', 'fakevm1')
+        redis.sadd('vmpooler__migration', 'fakevm2')
+        redis.sadd('vmpooler__migration', 'fakevm3')
+        redis.sadd('vmpooler__migration', 'fakevm4')
+        redis.sadd('vmpooler__migration', 'fakevm5')
+      end
+
+      it "should not migrate a VM if the migration limit is reached" do
+        expect(logger).to receive(:log).with('s',"[ ] [#{pool}] '#{vm}' is running on #{vm_parent_hostname}. No migration will be evaluated since the migration_limit has been reached")
+        subject._migrate_vm(vm, pool, vsphere)
+      end
+
+      it "should remove the VM from vmpooler__migrating queue in redis if the migration limit is reached" do
+        expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_truthy
+        subject._migrate_vm(vm, pool, vsphere)
+        expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_falsey
+      end
+    end
+
+    context 'when VM exists but migration limit is not yet reached' do
+      before(:each) do
+        expect(vsphere).to receive(:find_vm).with(vm).and_return(host)
+        allow(subject).to receive(:get_vm_host_info).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname])
+
+        create_migrating_vm(vm, pool)
+        redis.sadd('vmpooler__migration', 'fakevm1')
+        redis.sadd('vmpooler__migration', 'fakevm2')
+      end
+
+      context 'and host to migrate to is the same as the current host' do
+        before(:each) do
+          expect(vsphere).to receive(:find_least_used_compatible_host).with(host).and_return([{'name' => vm_parent_hostname}, vm_parent_hostname])
+        end
+
+        it "should not migrate the VM" do
+          expect(logger).to receive(:log).with('s', "[ ] [#{pool}] No migration required for '#{vm}' running on #{vm_parent_hostname}")
+          subject._migrate_vm(vm, pool, vsphere)
+        end
+
+        it "should remove the VM from vmpooler__migrating queue in redis" do
+          expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_truthy
+          subject._migrate_vm(vm, pool, vsphere)
+          expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_falsey
+        end
+
+        it "should not change the vmpooler_migration queue count" do
+          before_count = redis.scard('vmpooler__migration')
+          subject._migrate_vm(vm, pool, vsphere)
+          expect(redis.scard('vmpooler__migration')).to eq(before_count)
+        end
+
+        it "should call remove_vmpooler_migration_vm" do
+          expect(subject).to receive(:remove_vmpooler_migration_vm)
+          subject._migrate_vm(vm, pool, vsphere)
+        end
+      end
+
+      context 'and host to migrate to different to the current host' do
+        let(:vm_new_hostname) { 'new_hostname' }
+        before(:each) do
+          expect(vsphere).to receive(:find_least_used_compatible_host).with(host).and_return([{'name' => vm_new_hostname}, vm_new_hostname])
+          expect(subject).to receive(:migrate_vm_and_record_timing).with(host, vm, pool, Object, vm_parent_hostname, vm_new_hostname, vsphere).and_return('1.00')
+        end
+
+        it "should migrate the VM" do
+          expect(logger).to receive(:log).with('s', "[>] [#{pool}] '#{vm}' migrated from #{vm_parent_hostname} to #{vm_new_hostname} in 1.00 seconds")
+          subject._migrate_vm(vm, pool, vsphere)
+        end
+
+        it "should remove the VM from vmpooler__migrating queue in redis" do
+          expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_truthy
+          subject._migrate_vm(vm, pool, vsphere)
+          expect(redis.sismember("vmpooler__migrating__#{pool}",vm)).to be_falsey
+        end
+
+        it "should not change the vmpooler_migration queue count" do
+          before_count = redis.scard('vmpooler__migration')
+          subject._migrate_vm(vm, pool, vsphere)
+          expect(redis.scard('vmpooler__migration')).to eq(before_count)
+        end
+
+        it "should call remove_vmpooler_migration_vm" do
+          expect(subject).to receive(:remove_vmpooler_migration_vm)
+          subject._migrate_vm(vm, pool, vsphere)
+        end
+      end
+    end
+  end
+
   describe "#get_vm_host_info" do
     before do
       expect(subject).not_to be_nil