Adding many tests for #2371, and slightly refactoring

This mostly focuses on adding all of the detailed tests
for this new code, but it also cleans the code up
just a little bit.

Signed-off-by: Luke Kanies <luke@madstop.com>

2 files changed, 139 insertions, 47 deletions

diff --git a/lib/puppet/util/backups.rb b/lib/puppet/util/backups.rb
index b4bfbd261..4ab67771d 100644
--- a/lib/puppet/util/backups.rb
+++ b/lib/puppet/util/backups.rb
@@ -3,13 +3,14 @@ module Puppet::Util::Backups
 
     # Deal with backups.
     def perform_backup(file = nil)
-        # let the path be specified
-        file ||= self[:path]
-        return true unless FileTest.exists?(file)  
         # if they specifically don't want a backup, then just say
         # we're good
         return true unless self[:backup]
 
+        # let the path be specified
+        file ||= self[:path]
+        return true unless FileTest.exists?(file)  
+
         return perform_backup_with_bucket(file) if self.bucket
         return perform_backup_with_backuplocal(file, self[:backup]) 
     end
@@ -23,8 +24,7 @@ module Puppet::Util::Backups
             # we don't need to backup directories when recurse is on
             return true if self[:recurse]
             info "Recursively backing up to filebucket"
-            Find.find(self[:path]) { |f| backup_file_with_filebucket(f) if
-                File.file?(f) }
+            Find.find(self[:path]) { |f| backup_file_with_filebucket(f) if File.file?(f) }
         when "file"; backup_file_with_filebucket(file)
         when "link"; return true
         end
@@ -33,9 +33,9 @@ module Puppet::Util::Backups
     def perform_backup_with_backuplocal(fileobj, backup)
         file = (fileobj.class == String) ? fileobj : fileobj.name
         newfile = file + backup
-        if FileTest.exists?(newfile)
-            remove_backup(newfile)
-        end
+
+        remove_backup(newfile)
+
         begin
             bfile = file + backup
 
@@ -47,8 +47,7 @@ module Puppet::Util::Backups
         rescue => detail
             # since they said they want a backup, let's error out
             # if we couldn't make one
-            self.fail "Could not back %s up: %s" %
-                [file, detail.message]
+            self.fail "Could not back %s up: %s" % [file, detail.message]
         end
     end
 
@@ -58,23 +57,24 @@ module Puppet::Util::Backups
         else
             method = :stat
         end
-        old = File.send(method, newfile).ftype
 
-        if old == "directory"
-            raise Puppet::Error,
-            "Will not remove directory backup %s; use a filebucket" %
-                newfile
+        begin
+            stat = File.send(method, newfile)
+        rescue Errno::ENOENT
+            return
+        end
+
+        if stat.ftype == "directory"
+            raise Puppet::Error, "Will not remove directory backup %s; use a filebucket" % newfile
         end
 
-        info "Removing old backup of type %s" %
-            File.send(method, newfile).ftype
+        info "Removing old backup of type %s" % stat.ftype
 
         begin
             File.unlink(newfile)
         rescue => detail
             puts detail.backtrace if Puppet[:trace]
-            self.err "Could not remove old backup: %s" % detail
-            return false
+            self.fail "Could not remove old backup: %s" % detail
         end
     end
 
diff --git a/spec/unit/util/backups.rb b/spec/unit/util/backups.rb
index 2ddab5e85..c4bf26aef 100755
--- a/spec/unit/util/backups.rb
+++ b/spec/unit/util/backups.rb
@@ -7,62 +7,154 @@ require 'puppet/util/backups'
 include PuppetTest
 
 describe Puppet::Util::Backups do
+    before do
+        FileTest.stubs(:exists?).returns true
+    end
+
     describe "when backing up a file" do
-        it "should succeed silently if the file does not exist" do
-            Puppet::Type::File.new(:name => '/no/such/file').perform_backup.should be_true
+        it "should noop if the file does not exist" do
+            FileTest.expects(:exists?).returns false
+            file = Puppet::Type::File.new(:name => '/no/such/file')
+            file.expects(:bucket).never
+
+            file.perform_backup
         end
+
         it "should succeed silently if self[:backup] is false" do
-            FileTest.stubs(:exists?).returns true
-            Puppet::Type::File.new(:name => '/some/file', :backup => false).perform_backup.should be_true
+            file = Puppet::Type::File.new(:name => '/no/such/file', :backup => false)
+            file.expects(:bucket).never
+            FileTest.expects(:exists?).never
+            file.perform_backup
         end
-        it "a bucket should work when provided" do
+
+        it "a bucket should be used when provided" do
             path = '/my/file'
 
-            FileTest.stubs(:exists?).with(path).returns true
             File.stubs(:stat).with(path).returns(mock('stat', :ftype => 'file'))
 
-            bucket = mock('bucket', 'name' => 'foo')
-            bucket.expects(:backup).with(path)
-
             file = Puppet::Type::File.new(:name => path, :backup => 'foo')
+            bucket = stub('bucket', 'name' => 'foo')
             file.stubs(:bucket).returns bucket
 
-            file.perform_backup.should be_nil
+            bucket.expects(:backup).with(path).returns("mysum")
+
+            file.perform_backup
         end
-        it "a local backup should work" do
+
+        it "should propagate any exceptions encountered when backing up to a filebucket" do
             path = '/my/file'
-            FileTest.stubs(:exists?).with(path).returns true
 
-            file = Puppet::Type::File.new(:name => path, :backup => '.foo')
-            file.stubs(:perform_backup_with_backuplocal).returns true
-            file.perform_backup.should be_true
+            File.stubs(:stat).with(path).returns(mock('stat', :ftype => 'file'))
+
+            file = Puppet::Type::File.new(:name => path, :backup => 'foo')
+            bucket = stub('bucket', 'name' => 'foo')
+            file.stubs(:bucket).returns bucket
+
+            bucket.expects(:backup).raises ArgumentError
+
+            lambda { file.perform_backup }.should raise_error(ArgumentError)
+        end
+
+        describe "and no filebucket is configured" do
+            it "should remove any local backup if one exists" do
+                path = '/my/file'
+                FileTest.stubs(:exists?).returns true
+
+                backup = path + ".foo"
+
+                File.expects(:lstat).with(backup).returns stub("stat", :ftype => "file")
+                File.expects(:unlink).with(backup)
+
+                FileUtils.stubs(:cp_r)
+
+                file = Puppet::Type::File.new(:name => path, :backup => '.foo')
+                file.perform_backup
+            end
+
+            it "should fail when the old backup can't be removed" do
+                path = '/my/file'
+                FileTest.stubs(:exists?).returns true
+
+                backup = path + ".foo"
+
+                File.expects(:lstat).with(backup).returns stub("stat", :ftype => "file")
+                File.expects(:unlink).raises ArgumentError
+
+                FileUtils.expects(:cp_r).never
+
+                file = Puppet::Type::File.new(:name => path, :backup => '.foo')
+                lambda { file.perform_backup }.should raise_error(Puppet::Error)
+            end
+
+            it "should not try to remove backups that don't exist" do
+                path = '/my/file'
+                FileTest.stubs(:exists?).returns true
+
+                backup = path + ".foo"
+
+                File.expects(:lstat).with(backup).raises(Errno::ENOENT)
+                File.expects(:unlink).never
+
+                FileUtils.stubs(:cp_r)
+
+                file = Puppet::Type::File.new(:name => path, :backup => '.foo')
+                file.perform_backup
+            end
+
+            it "a copy should be created in the local directory" do
+                path = '/my/file'
+                FileTest.stubs(:exists?).with(path).returns true
+
+                FileUtils.expects(:cp_r).with(path, path + ".foo", :preserve => true)
+
+                file = Puppet::Type::File.new(:name => path, :backup => '.foo')
+                file.perform_backup.should be_true
+            end
+
+            it "should propagate exceptions if no backup can be created" do
+                path = '/my/file'
+                FileTest.stubs(:exists?).with(path).returns true
+
+                FileUtils.expects(:cp_r).raises ArgumentError
+
+                file = Puppet::Type::File.new(:name => path, :backup => '.foo')
+                lambda { file.perform_backup }.should raise_error(Puppet::Error)
+            end
         end
     end
+
     describe "when backing up a directory" do
         it "a bucket should work when provided" do
             path = '/my/dir'
 
-            FileTest.stubs(:exists?).with(path).returns true
-            File.stubs(:stat).with(path).returns(mock('stat', :ftype => 'directory'))
-            Find.stubs(:find).returns('')
+            File.stubs(:file?).returns true
+            Find.expects(:find).with(path).yields("/my/dir/file")
 
-            #bucket = mock('bucket', 'name' => 'foo')
-            bucket = mock('bucket')
-            bucket.stubs(:backup).with(path).returns true
+            bucket = stub('bucket', :name => "eh")
+            bucket.expects(:backup).with("/my/dir/file").returns true
 
             file = Puppet::Type::File.new(:name => path, :backup => 'foo')
             file.stubs(:bucket).returns bucket
 
-            file.perform_backup.should be_true
+            File.stubs(:stat).with(path).returns(stub('stat', :ftype => 'directory'))
+
+            file.perform_backup
         end
-        it "a local backup should work" do
+
+        it "should do nothing when recursing" do
             path = '/my/dir'
-            FileTest.stubs(:exists?).with(path).returns true
 
-            file = Puppet::Type::File.new(:name => path, :backup => '.foo')
-            file.stubs(:perform_backup_with_backuplocal).returns true
-            file.perform_backup.should be_true
+            bucket = stub('bucket', :name => "eh")
+            bucket.expects(:backup).never
+
+            file = Puppet::Type::File.new(:name => path, :backup => 'foo', :recurse => true)
+            file.stubs(:bucket).returns bucket
+
+            File.stubs(:stat).with(path).returns(stub('stat', :ftype => 'directory'))
+
+            Find.expects(:find).never
+
+            file.perform_backup
         end
     end
 end
-