Cleaning up tests for #2469

This is a small refactor of the filebucket type, and a
larger refactor of the tests.

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

2 files changed, 70 insertions, 43 deletions

diff --git a/lib/puppet/type/filebucket.rb b/lib/puppet/type/filebucket.rb
index f0d2c8e82..468f92638 100755
--- a/lib/puppet/type/filebucket.rb
+++ b/lib/puppet/type/filebucket.rb
@@ -38,13 +38,14 @@ module Puppet
                   specified then *path* is checked. If it is set, then the
                   bucket is local.  Otherwise the puppetmaster server specified
                   in the config or at the commandline is used."
+            defaultto { Puppet[:server] }
         end
 
         newparam(:port) do
             desc "The port on which the remote server is listening.
                 Defaults to the normal Puppet port, %s." % Puppet[:masterport]
 
-            defaultto Puppet[:masterport]
+            defaultto { Puppet[:masterport] }
         end
 
         newparam(:path) do
@@ -60,50 +61,32 @@ module Puppet
             new(:name => "puppet", :path => Puppet[:clientbucketdir])
         end
 
-        def self.instances
-            []
-        end
-
         def bucket
-            unless defined? @bucket
-                mkbucket()
-            end
-
-            @bucket
+            mkbucket() unless defined? @bucket
+            return @bucket
         end
 
+        private
+
         def mkbucket
             # Default is a local filebucket, if no server is given.
             # If the default path has been removed, too, then
             # the puppetmaster is used as default server
 
             type = "local"
-            if self[:server]
-                type = "remote"
-                server = self[:server]
-            elsif not self[:path]
-                type = "remote"
-                server = Puppet[:server]
+            args = {}
+            if self[:path]
+                args[:Path] = self[:path]
+            else
+                args[:Server] = self[:server]
+                args[:Port] = self[:port]
             end
 
             begin
-                if type == "local"
-                    @bucket = Puppet::Network::Client.client(:Dipper).new(
-                        :Path => self[:path]
-                    )
-                else
-                    @bucket = Puppet::Network::Client.client(:Dipper).new(
-                        :Server => server,
-                        :Port => self[:port]
-                    )
-                end
+                @bucket = Puppet::Network::Client.client(:Dipper).new(args)
             rescue => detail
-                if Puppet[:trace]
-                    puts detail.backtrace
-                end
-                self.fail(
-                    "Could not create %s filebucket: %s" % [type, detail]
-                )
+                puts detail.backtrace if Puppet[:trace]
+                self.fail("Could not create %s filebucket: %s" % [type, detail])
             end
 
             @bucket.name = self.name
diff --git a/spec/unit/type/filebucket.rb b/spec/unit/type/filebucket.rb
index 9a635d517..78bfa3655 100644
--- a/spec/unit/type/filebucket.rb
+++ b/spec/unit/type/filebucket.rb
@@ -3,28 +3,72 @@
 require File.dirname(__FILE__) + '/../../spec_helper'
 
 describe Puppet::Type.type(:filebucket) do
+    describe "when validating attributes" do
+        %w{name server port path}.each do |attr|
+            it "should have a '#{attr}' parameter" do
+                Puppet::Type.type(:filebucket).attrtype(attr.intern).should == :param
+            end
+        end
+
+        it "should have its 'name' attribute set as its namevar" do
+            Puppet::Type.type(:filebucket).namevar.should == :name
+        end
+    end
+
+    it "should use the clientbucketdir as the path by default path" do
+        Puppet.settings[:clientbucketdir] = "/my/bucket"
+        Puppet::Type.type(:filebucket).new(:name => "main")[:path].should == Puppet[:clientbucketdir]
+    end
+
+    it "should use the masterport as the path by default port" do
+        Puppet.settings[:masterport] = 50
+        Puppet::Type.type(:filebucket).new(:name => "main")[:port].should == Puppet[:masterport]
+    end
+
+    it "should use the server as the path by default server" do
+        Puppet.settings[:server] = "myserver"
+        Puppet::Type.type(:filebucket).new(:name => "main")[:server].should == Puppet[:server]
+    end
+
     it "be local by default" do
         bucket = Puppet::Type.type(:filebucket).new :name => "main"
 
-        bucket.name.should == "main"
-        bucket.bucket.should be_instance_of(Puppet::Network::Client::Dipper)
-        bucket.bucket.local.should == true
+        bucket.bucket.should be_local
     end
 
     it "not be local if path is false" do
         bucket = Puppet::Type.type(:filebucket).new :name => "main", :path => false
 
-        bucket.name.should == "main"
-        bucket.bucket.should be_instance_of(Puppet::Network::Client::Dipper)
-        bucket.bucket.local.should_not == true
+        bucket.bucket.should_not be_local
     end
 
-    it "not be local if a server is specified" do
-        bucket = Puppet::Type.type(:filebucket).new :name => "main", :server => "puppet"
+    it "be local if both a path and a server are specified" do
+        bucket = Puppet::Type.type(:filebucket).new :name => "main", :server => "puppet", :path => "/my/path"
 
-        bucket.name.should == "main"
-        bucket.bucket.should be_instance_of(Puppet::Network::Client::Dipper)
-        bucket.bucket.local.should_not == true
+        bucket.bucket.should be_local
     end
 
+    describe "when creating the filebucket" do
+        before do
+            @bucket = stub 'bucket', :name= => nil
+        end
+
+        it "should use any provided path" do
+            bucket = Puppet::Type.type(:filebucket).new :name => "main", :path => "/foo/bar"
+            Puppet::Network::Client.client(:Dipper).expects(:new).with(:Path => "/foo/bar").returns @bucket
+            bucket.bucket
+        end
+        it "should use any provided server and port" do
+            bucket = Puppet::Type.type(:filebucket).new :name => "main", :server => "myserv", :port => "myport", :path => false
+            Puppet::Network::Client.client(:Dipper).expects(:new).with(:Server => "myserv", :Port => "myport").returns @bucket
+            bucket.bucket
+        end
+
+        it "should use the default server if the path is unset and no server is provided" do
+            Puppet.settings[:server] = "myserv"
+            bucket = Puppet::Type.type(:filebucket).new :name => "main", :path => false
+            Puppet::Network::Client.client(:Dipper).expects(:new).with { |args| args[:Server] == "myserv" }.returns @bucket
+            bucket.bucket
+        end
+    end
 end