Fix bug in test, add more specs and small refactor

The tests were failing when run on a machine with certs on the file system
Stub out failure to read where appropriate

Worked fine at my desk :(

2 files changed, 86 insertions, 35 deletions

diff --git a/lib/puppet/executables/client/certhandler.rb b/lib/puppet/executables/client/certhandler.rb
index d2ead3950..b041397ae 100644
--- a/lib/puppet/executables/client/certhandler.rb
+++ b/lib/puppet/executables/client/certhandler.rb
@@ -4,13 +4,21 @@ module Puppet
         module Client
             class CertHandler
                 attr_writer :wait_for_cert, :one_time
+                attr_reader :new_cert
                 
                 def initialize(wait_time, is_one_time)
                     @wait_for_cert = wait_time
                     @one_time = is_one_time
                     @new_cert = false
                 end
+
+                # Did we just read a cert?
+                def new_cert?
+                    new_cert
+                end
                 
+                # Read, or retrieve if necessary, our certificate.  Returns true if we retrieved
+                # a new cert, false if the cert already exists.
                 def read_retrieve 
                     #NOTE: ACS this is checking that a file exists, maybe next time just do that?
                     unless read_cert 
@@ -19,7 +27,7 @@ module Puppet
                         retrieve_cert
                     end
 
-                    !@new_cert
+                    ! new_cert?
                 end
 
                 def retrieve_cert
@@ -46,13 +54,14 @@ module Puppet
                 end
 
                 def read_cert
-                     Puppet::Network::HttpPool.read_cert
+                    Puppet::Network::HttpPool.read_cert
                 end
 
                 def read_new_cert
                     if Puppet::Network::HttpPool.read_cert
                         # If we read it in, then we need to get rid of our existing http connection.
-                        # The @new_cert flag will help us do that
+                        # The @new_cert flag will help us do that, in that it provides a way
+                        # to notify that the cert status has changed.
                         @new_cert = true
                         Puppet.notice "Got signed certificate"
                     else
diff --git a/spec/unit/executables/client/certhandler.rb b/spec/unit/executables/client/certhandler.rb
index 0a7b77f15..4f8f8139c 100755
--- a/spec/unit/executables/client/certhandler.rb
+++ b/spec/unit/executables/client/certhandler.rb
@@ -14,67 +14,109 @@ describe cert_handler, "when handling certificates" do
         Puppet::Network::Client.stubs(:ca).returns(caclient_class)
     end
 
-    it "should return true if the certificate exists" do
-        Puppet::Network::HttpPool.expects(:read_cert).returns(true)
-        cert_handler.new(1,true).read_retrieve.should be_true
-    end
-    
-    it "should return false when getting a new cert" do
-        Puppet::Network::HttpPool.expects(:read_cert).returns(true)
-        @caclient.stubs(:request_cert).returns(true)
-        ch = cert_handler.new(1,true)
-        ch.stubs(:read_cert).returns(false)
-        ch.read_retrieve.should be_false
+    describe "when reading or retrieving the certificate" do
+        before do
+            @handler = cert_handler.new(1,true)
+        end
+
+        it "should attempt to read the certificate" do
+            @handler.expects(:read_cert).returns true
+            @handler.read_retrieve
+        end
+
+        it "should delegate to Puppet::Network::HttpPool to read the certificate" do
+            Puppet::Network::HttpPool.expects(:read_cert).returns(true)
+            @handler.read_retrieve
+        end
+
+        it "should not attempt to retrieve a certificate if one can be read" do
+            @handler.stubs(:read_cert).returns true
+            @handler.expects(:retrieve_cert).never
+            @handler.read_retrieve
+        end
+
+        it "should attempt to retrieve a certificate if none can be read" do
+            @handler.stubs(:read_cert).returns false
+            @handler.expects(:retrieve_cert)
+            @handler.read_retrieve
+        end
+
+        it "should delegate to caclient to retrieve a certificate" do
+            @handler.stubs(:read_cert).returns false
+            @caclient.expects(:request_cert).returns(true)
+            @handler.stubs(:read_new_cert).returns(true)
+            @handler.read_retrieve
+        end
+
+        it "should return true if the certificate exists" do
+            @handler.stubs(:read_cert).returns true
+            @handler.read_retrieve.should be_true
+        end
+
+        it "should return false when getting a new cert" do
+            #This is the second call to httppool that happens in 'read_new_cert'
+            Puppet::Network::HttpPool.expects(:read_cert).returns(true)
+            @caclient.stubs(:request_cert).returns(true)
+            @handler.stubs(:read_cert).returns(false)
+            @handler.read_retrieve.should be_false
+        end
     end
 
     describe "when waiting for cert" do
+        before do
+            @handler = cert_handler.new(1,false)
+            @handler.stubs(:read_cert).returns false
+            #all waiting for cert tests should loop, which will always happen if sleep is called
+            #yeah, I put the expectation in the setup, deal with it
+            @handler.expects(:sleep).with(1)
+
+            #This is needed to get out of the loop
+            @handler.stubs(:read_new_cert).returns(true)
+        end
+
         it "should loop when the cert request does not return a certificate" do
             @caclient.stubs(:request_cert).times(2).returns(false).then.returns(true)
-            ch = cert_handler.new(1,false)
-            ch.expects(:sleep)
-            ch.expects(:read_new_cert).returns(true)
-            ch.read_retrieve
+            @handler.retrieve_cert
         end
 
         it "should loop when the cert request raises an Error" do
             @caclient.stubs(:request_cert).times(2).raises(StandardError, 'Testing').then.returns(true)
-            ch = cert_handler.new(1,false)
-            ch.expects(:sleep)
-            ch.expects(:read_new_cert).returns(true)
-            ch.read_retrieve
+            @handler.retrieve_cert
         end
         
         it "should loop when the new cert can't be read" do
             @caclient.stubs(:request_cert).returns(true)
-            ch = cert_handler.new(1,false)
-            ch.expects(:sleep)
-            ch.expects(:read_new_cert).times(2).returns(false).then.returns(true)
-            ch.read_retrieve
+            @handler.stubs(:read_new_cert).times(2).returns(false).then.returns(true)
+            @handler.retrieve_cert
         end
     end
 
     describe "when in one time mode" do
+        before do
+            #true puts us in onetime mode
+            @handler = cert_handler.new(1,true)
+            @handler.stubs(:read_cert).returns false
+        end
+
         it "should exit if the cert request does not return a certificate" do
             @caclient.stubs(:request_cert).returns(false)
-            ch = cert_handler.new(1,true)
-            ch.expects(:exit).with(1).raises(SystemExit)
-            lambda { ch.read_retrieve }.should raise_error(SystemExit)
+            @handler.expects(:exit).with(1).raises(SystemExit)
+            lambda { @handler.retrieve_cert }.should raise_error(SystemExit)
         end
 
 
         it "should exit if the cert request raises an exception" do
             @caclient.stubs(:request_cert).raises(StandardError, 'Testing')
-            ch = cert_handler.new(1,true)
-            ch.expects(:exit).with(23).raises(SystemExit)
-            lambda { ch.read_retrieve }.should raise_error(SystemExit)
+            @handler.expects(:exit).with(23).raises(SystemExit)
+            lambda { @handler.retrieve_cert }.should raise_error(SystemExit)
         end
         
         it "should exit if the new cert can't be read" do
             @caclient.stubs(:request_cert).returns(true)
+            #this is the second, call to httppool inside read_new_cert
             Puppet::Network::HttpPool.stubs(:read_cert).returns(false)
-            ch = cert_handler.new(1,true)
-            ch.expects(:exit).with(34).raises(SystemExit)
-            lambda { ch.read_retrieve }.should raise_error(SystemExit)
+            @handler.expects(:exit).with(34).raises(SystemExit)
+            lambda { @handler.retrieve_cert }.should raise_error(SystemExit)
         end
     end
 end