Moving fact and plugin handling into modules

This doesn't change functionality, it just simplifies
the agent class.

I've also started the work to get the catalog handling
done using REST/the Indirector.

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

4 files changed, 421 insertions, 232 deletions

diff --git a/spec/unit/agent.rb b/spec/unit/agent.rb
index dc2fc9034..c204fa8fb 100755
--- a/spec/unit/agent.rb
+++ b/spec/unit/agent.rb
@@ -6,323 +6,295 @@
 require File.dirname(__FILE__) + '/../spec_helper'
 require 'puppet/agent'
 
-describe Puppet::Agent, " when retrieving the catalog" do
-    before do
-        Puppet.settings.stubs(:use).returns(true)
-        @client = Puppet::Agent.new
-        @facts = {"one" => "two", "three" => "four"}
+describe Puppet::Agent do
+    it "should include the Plugin Handler module" do
+        Puppet::Agent.ancestors.should be_include(Puppet::Agent::PluginHandler)
     end
 
-    it "should initialize the metadata store" do
-        @client.class.stubs(:facts).returns(@facts)
-        @client.expects(:dostorage)
-        @master.stubs(:getconfig).returns(nil)
-        @client.getconfig
-    end
-
-    it "should collect facts to use for catalog retrieval" do
-        @client.stubs(:dostorage)
-        @client.class.expects(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns(nil)
-        @client.getconfig
-    end
-
-    it "should fail if no facts could be collected" do
-        @client.stubs(:dostorage)
-        @client.class.expects(:facts).returns({})
-        @master.stubs(:getconfig).returns(nil)
-        proc { @client.getconfig }.should raise_error(Puppet::Error)
-    end
-
-    it "should retrieve plugins if :pluginsync is enabled" do
-        file = "/path/to/cachefile"
-        @client.stubs(:cachefile).returns(file)
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        Puppet.settings.expects(:value).with(:pluginsync).returns(true)
-        @client.expects(:getplugins)
-        @client.stubs(:get_actual_config).returns(nil)
-        FileTest.stubs(:exist?).with(file).returns(true)
-        @client.stubs(:use_cached_config).returns(true)
-        @client.class.stubs(:facts).returns(@facts)
-        @client.getconfig
-    end
-
-    it "should use the cached catalog if no catalog could be retrieved" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).raises(ArgumentError.new("whev"))
-        @client.expects(:use_cached_config).with(true)
-        @client.getconfig
-    end
-
-    describe "when the catalog format is set to yaml" do
-        before do
-            Puppet.settings.stubs(:value).returns "foo"
-            Puppet.settings.stubs(:value).with(:pluginsync).returns false
-            Puppet.settings.stubs(:value).with(:configtimeout).returns 10
-            Puppet.settings.stubs(:value).with(:factsync).returns false
-            Puppet.settings.stubs(:value).with(:catalog_format).returns "yaml"
-        end
-
-        it "should request a yaml-encoded catalog" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.expects(:getconfig).with { |*args| args[1] == "yaml" }
+    it "should include the Fact Handler module" do
+        Puppet::Agent.ancestors.should be_include(Puppet::Agent::FactHandler)
+    end
+end
 
-            @client.getconfig
-        end
+describe Puppet::Agent, "when executing a catalog run" do
+    before do
+        Puppet.settings.stubs(:use).returns(true)
+        @agent = Puppet::Agent.new
+        @agent.stubs(:splay)
 
-        it "should load the retrieved catalog using YAML" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.stubs(:getconfig).returns("myconfig")
+        @lockfile = stub 'lockfile', :lock => true, :locked? => false, :lockfile => "/my/lock/file", :unlock => true
 
-            config = mock 'config'
-            YAML.expects(:load).with("myconfig").returns(config)
+        @agent.stubs(:lockfile).returns @lockfile
+    end
 
-            @client.stubs(:setclasses)
+    it "should splay" do
+        Puppet::Util.sync(:puppetrun).stubs(:synchronize)
+        @agent.expects(:splay)
+        @agent.run
+    end
 
-            config.stubs(:classes)
-            config.stubs(:to_catalog).returns(config)
-            config.stubs(:host_config=)
-            config.stubs(:from_cache).returns(true)
+    it "should use a global mutex to make sure no other thread is executing the catalog" do
+        sync = mock 'sync'
+        Puppet::Util.expects(:sync).with(:puppetrun).returns sync
 
-            @client.getconfig
-        end
+        sync.expects(:synchronize)
 
-        it "should use the cached catalog if the retrieved catalog cannot be converted from YAML" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.stubs(:getconfig).returns("myconfig")
+        @agent.expects(:retrieve_config).never # i.e., if we don't yield, we don't retrieve the config
+        @agent.run
+    end
 
-            YAML.expects(:load).with("myconfig").raises(ArgumentError)
+    it "should use a lockfile to make sure no other process is executing the catalog" do
+        @lockfile.expects(:lock).returns true
 
-            @client.expects(:use_cached_config).with(true)
+        @agent.expects(:retrieve_catalog)
 
-            @client.getconfig
-        end
+        @agent.run
     end
 
-    describe "from Marshal" do
-        before do
-            Puppet.settings.stubs(:value).returns "foo"
-            Puppet.settings.stubs(:value).with(:pluginsync).returns false
-            Puppet.settings.stubs(:value).with(:configtimeout).returns 10
-            Puppet.settings.stubs(:value).with(:factsync).returns false
-            Puppet.settings.stubs(:value).with(:catalog_format).returns "marshal"
-        end
+    it "should log and do nothing if the lock cannot be acquired" do
+        @lockfile.expects(:lock).returns false
 
-        it "should load the retrieved catalog using Marshal" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.stubs(:getconfig).returns("myconfig")
+        @agent.expects(:retrieve_catalog).never
 
-            config = mock 'config'
-            Marshal.expects(:load).with("myconfig").returns(config)
+        Puppet.expects(:notice)
 
-            @client.stubs(:setclasses)
+        @agent.run
+    end
 
-            config.stubs(:classes)
-            config.stubs(:to_catalog).returns(config)
-            config.stubs(:host_config=)
-            config.stubs(:from_cache).returns(true)
+    it "should retrieve the catalog" do
+        @agent.expects(:retrieve_catalog)
 
-            @client.getconfig
-        end
+        @agent.run
+    end
 
-        it "should use the cached catalog if the retrieved catalog cannot be converted from Marshal" do
-            @client.stubs(:dostorage)
-            @client.class.stubs(:facts).returns(@facts)
-            @master.stubs(:getconfig).returns("myconfig")
+    it "should log a failure and do nothing if no catalog can be retrieved" do
+        @agent.expects(:retrieve_catalog).returns nil
 
-            Marshal.expects(:load).with("myconfig").raises(ArgumentError)
+        Puppet.expects(:err)
 
-            @client.expects(:use_cached_config).with(true)
+        @agent.run
+    end
 
-            @client.getconfig
-        end
+    it "should apply the catalog with all options to :run" do
+        catalog = stub 'catalog', :retrieval_duration= => nil
+        @agent.expects(:retrieve_catalog).returns catalog
+
+        catalog.expects(:apply).with(:one => true)
+        @agent.run :one => true
     end
+    
+    it "should benchmark how long it takes to apply the catalog" do
+        @agent.expects(:benchmark).with(:notice, "Finished catalog run")
 
-    it "should set the classes.txt file with the classes listed in the retrieved catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+        catalog = stub 'catalog', :retrieval_duration= => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        config = mock 'config'
-        YAML.expects(:load).with("myconfig").returns(config)
+        catalog.expects(:apply).never # because we're not yielding
+        @agent.run
+    end
+
+    it "should remove the lock file when done applying the catalog" do
+        catalog = stub 'catalog', :retrieval_duration= => nil, :apply => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        config.expects(:classes).returns(:myclasses)
-        @client.expects(:setclasses).with(:myclasses)
+        @lockfile.expects(:lock).returns true
 
-        config.stubs(:to_catalog).returns(config)
-        config.stubs(:host_config=)
-        config.stubs(:from_cache).returns(true)
+        @lockfile.expects(:unlock)
 
-        @client.getconfig
+        @agent.run
     end
 
-    it "should convert the retrieved catalog to a RAL catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    it "should remove the lock file even if there was an exception during the run" do
+        catalog = stub 'catalog', :retrieval_duration= => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+        catalog.expects(:apply).raises "eh"
 
-        @client.stubs(:setclasses)
+        @lockfile.expects(:unlock)
+        @agent.run
+    end
 
-        config = mock 'config'
+    it "should HUP itself if it should be restarted" do
+        catalog = stub 'catalog', :retrieval_duration= => nil, :apply => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.expects(:to_catalog).returns(config)
+        Process.expects(:kill).with(:HUP, $$)
 
-        config.stubs(:host_config=)
-        config.stubs(:from_cache).returns(true)
+        @agent.expects(:restart?).returns true
 
-        @client.getconfig
+        @agent.run
     end
 
-    it "should use the cached catalog if the retrieved catalog cannot be converted to a RAL catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    it "should not HUP itself if it should not be restarted" do
+        catalog = stub 'catalog', :retrieval_duration= => nil, :apply => nil
+        @agent.expects(:retrieve_catalog).returns catalog
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+        Process.expects(:kill).never
 
-        @client.stubs(:setclasses)
+        @agent.expects(:restart?).returns false
 
-        config = mock 'config'
+        @agent.run
+    end
+end
+
+describe Puppet::Agent, "when retrieving a catalog" do
+    before do
+        Puppet.settings.stubs(:use).returns(true)
+        @agent = Puppet::Agent.new
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.expects(:to_catalog).raises(ArgumentError)
+        @catalog = stub 'catalog', :retrieval_duration= => nil
+    end
 
-        @client.expects(:use_cached_config).with(true)
+    it "should use the Catalog class to get its catalog" do
+        Puppet::Resource::Catalog.expects(:get).returns @catalog
 
-        @client.getconfig
+        @agent.retrieve_catalog
     end
 
-    it "should clear the failed catalog if using the cached catalog after failing to instantiate the retrieved catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    it "should use its Facter name to retrieve the catalog" do
+        Facter.stubs(:value).returns "eh"
+        Facter.expects(:value).with("hostname").returns "myhost"
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| name == "myhost" }.returns @catalog
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+        @agent.retrieve_catalog
+    end
 
-        @client.stubs(:setclasses)
+    it "should default to returning a catalog retrieved directly from the server, skipping the cache" do
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == false }.returns @catalog
 
-        config = mock 'config'
+        @agent.retrieve_catalog.should == @catalog
+    end
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.stubs(:to_catalog).raises(ArgumentError)
+    it "should return the cached catalog when no catalog can be retrieved from the server" do
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == false }.returns nil
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == true }.returns @catalog
 
-        @client.stubs(:use_cached_config).with(true)
+        @agent.retrieve_catalog.should == @catalog
+    end
 
-        @client.expects(:clear)
+    it "should return the cached catalog when retrieving the remote catalog throws an exception" do
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == false }.raises "eh"
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == true }.returns @catalog
 
-        @client.getconfig
+        @agent.retrieve_catalog.should == @catalog
     end
 
-    it "should cache the retrieved yaml catalog if it is not from the cache and is valid" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+    it "should return nil if no cached catalog is available and no catalog can be retrieved from the server" do
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == false }.returns nil
+        Puppet::Resource::Catalog.expects(:get).with { |name, options| options[:use_cache] == true }.returns nil
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+        @agent.retrieve_catalog.should be_nil
+    end
 
-        @client.stubs(:setclasses)
+    it "should record the retrieval time with the catalog" do
+        @agent.expects(:thinmark).yields.then.returns 10
 
-        config = mock 'config'
+        catalog = mock 'catalog'
+        Puppet::Resource::Catalog.expects(:get).returns catalog
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.expects(:to_catalog).returns(config)
+        catalog.expects(:retrieval_duration=).with 10
 
-        config.stubs(:host_config=)
+        @agent.retrieve_catalog
+    end
 
-        config.expects(:from_cache).returns(false)
+    it "should update the class file with the classes contained within the catalog"
 
-        @client.expects(:cache).with("myconfig")
+    it "should mark the catalog as a host catalog"
 
-        @client.getconfig
-    end
+    it "should return nil if there is an error while retrieving the catalog" do
+        Puppet::Resource::Catalog.expects(:get).raises "eh"
 
-    it "should mark the catalog as a host catalog" do
-        @client.stubs(:dostorage)
-        @client.class.stubs(:facts).returns(@facts)
-        @master.stubs(:getconfig).returns("myconfig")
+        @agent.retrieve_catalog.should be_nil
+    end
+end
 
-        yamlconfig = mock 'yaml config'
-        YAML.stubs(:load).returns(yamlconfig)
+describe Puppet::Agent, "when preparing for a run" do
+    before do
+        Puppet.settings.stubs(:use).returns(true)
+        @agent = Puppet::Agent.new
+        @agent.stubs(:dostorage)
+        @agent.stubs(:upload_facts)
+        @facts = {"one" => "two", "three" => "four"}
+    end
 
-        @client.stubs(:setclasses)
+    it "should initialize the metadata store" do
+        @agent.class.stubs(:facts).returns(@facts)
+        @agent.expects(:dostorage)
+        @agent.prepare
+    end
 
-        config = mock 'config'
+    it "should download fact plugins" do
+        @agent.stubs(:dostorage)
+        @agent.expects(:download_fact_plugins)
 
-        yamlconfig.stubs(:classes)
-        yamlconfig.expects(:to_catalog).returns(config)
+        @agent.prepare
+    end
 
-        config.stubs(:from_cache).returns(true)
+    it "should download plugins" do
+        @agent.stubs(:dostorage)
+        @agent.expects(:download_plugins)
 
-        config.expects(:host_config=).with(true)
+        @agent.prepare
+    end
 
-        @client.getconfig
+    it "should upload facts to use for catalog retrieval" do
+        @agent.stubs(:dostorage)
+        @agent.expects(:upload_facts)
+        @agent.prepare
     end
 end
 
 describe Puppet::Agent, " when using the cached catalog" do
     before do
         Puppet.settings.stubs(:use).returns(true)
-        @client = Puppet::Agent.new
+        @agent = Puppet::Agent.new
         @facts = {"one" => "two", "three" => "four"}
     end
 
     it "should return do nothing and true if there is already an in-memory catalog" do
-        @client.catalog = :whatever
+        @agent.catalog = :whatever
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config.should be_true
+            @agent.use_cached_config.should be_true
         end
     end
 
     it "should return do nothing and false if it has been told there is a failure and :nocacheonfailure is enabled" do
         Puppet.settings.expects(:value).with(:usecacheonfailure).returns(false)
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config(true).should be_false
+            @agent.use_cached_config(true).should be_false
         end
     end
 
     it "should return false if no cached catalog can be found" do
-        @client.expects(:retrievecache).returns(nil)
+        @agent.expects(:retrievecache).returns(nil)
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_false
+            @agent.use_cached_config().should be_false
         end
     end
 
     it "should return false if the cached catalog cannot be instantiated" do
         YAML.expects(:load).raises(ArgumentError)
-        @client.expects(:retrievecache).returns("whatever")
+        @agent.expects(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_false
+            @agent.use_cached_config().should be_false
         end
     end
 
     it "should warn if the cached catalog cannot be instantiated" do
         YAML.stubs(:load).raises(ArgumentError)
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet.expects(:warning).with { |m| m.include?("Could not load cache") }
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_false
+            @agent.use_cached_config().should be_false
         end
     end
 
     it "should clear the client if the cached catalog cannot be instantiated" do
         YAML.stubs(:load).raises(ArgumentError)
-        @client.stubs(:retrievecache).returns("whatever")
-        @client.expects(:clear)
+        @agent.stubs(:retrievecache).returns("whatever")
+        @agent.expects(:clear)
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_false
+            @agent.use_cached_config().should be_false
         end
     end
 
@@ -335,9 +307,9 @@ describe Puppet::Agent, " when using the cached catalog" do
         ral_config.stubs(:host_config=)
         config.expects(:to_catalog).returns(ral_config)
 
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config().should be_true
+            @agent.use_cached_config().should be_true
         end
     end
 
@@ -350,12 +322,12 @@ describe Puppet::Agent, " when using the cached catalog" do
         ral_config.stubs(:host_config=)
         config.expects(:to_catalog).returns(ral_config)
 
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config()
+            @agent.use_cached_config()
         end
 
-        @client.catalog.should equal(ral_config)
+        @agent.catalog.should equal(ral_config)
     end
 
     it "should mark the catalog as a host_config if valid" do
@@ -367,12 +339,12 @@ describe Puppet::Agent, " when using the cached catalog" do
         ral_config.expects(:host_config=).with(true)
         config.expects(:to_catalog).returns(ral_config)
 
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config()
+            @agent.use_cached_config()
         end
 
-        @client.catalog.should equal(ral_config)
+        @agent.catalog.should equal(ral_config)
     end
 
     it "should mark the catalog as from the cache if valid" do
@@ -384,19 +356,19 @@ describe Puppet::Agent, " when using the cached catalog" do
         ral_config.stubs(:host_config=)
         config.expects(:to_catalog).returns(ral_config)
 
-        @client.stubs(:retrievecache).returns("whatever")
+        @agent.stubs(:retrievecache).returns("whatever")
         Puppet::Agent.publicize_methods :use_cached_config do
-            @client.use_cached_config()
+            @agent.use_cached_config()
         end
 
-        @client.catalog.should equal(ral_config)
+        @agent.catalog.should equal(ral_config)
     end
 
     describe "when calling splay" do
         it "should do nothing if splay is not enabled" do
             Puppet.stubs(:[]).with(:splay).returns(false)
-            @client.expects(:rand).never
-            @client.send(:splay)
+            @agent.expects(:rand).never
+            @agent.send(:splay)
         end
 
         describe "when splay is enabled" do
@@ -406,30 +378,30 @@ describe Puppet::Agent, " when using the cached catalog" do
             end
 
             it "should sleep for a random time plus 1" do
-                @client.expects(:rand).with(43).returns(43)
-                @client.expects(:sleep).with(43)
-                @client.send(:splay)
+                @agent.expects(:rand).with(43).returns(43)
+                @agent.expects(:sleep).with(43)
+                @agent.send(:splay)
             end
 
             it "should inform that it is splayed" do
-                @client.stubs(:rand).with(43).returns(43)
-                @client.stubs(:sleep).with(43)
+                @agent.stubs(:rand).with(43).returns(43)
+                @agent.stubs(:sleep).with(43)
                 Puppet.expects(:info)
-                @client.send(:splay)
+                @agent.send(:splay)
             end
 
             it "should set splay = true" do
-                @client.stubs(:rand).returns(43)
-                @client.stubs(:sleep)
-                @client.send(:splay)
-                @client.send(:splayed?).should == true
+                @agent.stubs(:rand).returns(43)
+                @agent.stubs(:sleep)
+                @agent.send(:splay)
+                @agent.send(:splayed?).should == true
             end
 
             it "should do nothing if already splayed" do
-                @client.stubs(:rand).returns(43).at_most_once
-                @client.stubs(:sleep).at_most_once
-                @client.send(:splay)
-                @client.send(:splay)
+                @agent.stubs(:rand).returns(43).at_most_once
+                @agent.stubs(:sleep).at_most_once
+                @agent.send(:splay)
+                @agent.send(:splay)
             end
         end 
     end
diff --git a/spec/unit/agent/downloader.rb b/spec/unit/agent/downloader.rb
index 6b07e5bb4..28c5d030f 100755
--- a/spec/unit/agent/downloader.rb
+++ b/spec/unit/agent/downloader.rb
@@ -99,7 +99,7 @@ describe Puppet::Agent::Downloader do
 
             @dler.expects(:file).returns file
 
-            Puppet::Node::Catalog.expects(:new).returns catalog
+            Puppet::Resource::Catalog.expects(:new).returns catalog
             catalog.expects(:add_resource).with(file)
 
             @dler.catalog.should equal(catalog)
diff --git a/spec/unit/agent/fact_handler.rb b/spec/unit/agent/fact_handler.rb
new file mode 100755
index 000000000..b58f55ebc
--- /dev/null
+++ b/spec/unit/agent/fact_handler.rb
@@ -0,0 +1,117 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+require 'puppet/agent'
+require 'puppet/agent/fact_handler'
+
+class FactHandlerTester
+    include Puppet::Agent::FactHandler
+end
+
+describe Puppet::Agent::FactHandler do
+    before do
+        @facthandler = FactHandlerTester.new
+    end
+
+    it "should have a method for downloading fact plugins" do
+        @facthandler.should respond_to(:download_fact_plugins)
+    end
+
+    it "should have a boolean method for determining whether fact plugins should be downloaded" do
+        @facthandler.should respond_to(:download_fact_plugins?)
+    end
+
+    it "should download fact plugins when :factsync is true" do
+        Puppet.settings.expects(:value).with(:factsync).returns true
+        @facthandler.should be_download_fact_plugins
+    end
+
+    it "should not download fact plugins when :factsync is false" do
+        Puppet.settings.expects(:value).with(:factsync).returns false
+        @facthandler.should_not be_download_fact_plugins
+    end
+
+    it "should not download fact plugins when downloading is disabled" do
+        Puppet::Agent::Downloader.expects(:new).never
+        @facthandler.expects(:download_fact_plugins?).returns false
+        @facthandler.download_fact_plugins
+    end
+
+    it "should use an Agent Downloader, with the name, source, destination, and ignore set correctly, to download fact plugins when downloading is enabled" do
+        downloader = mock 'downloader'
+
+        Puppet.settings.expects(:value).with(:factsource).returns "fsource"
+        Puppet.settings.expects(:value).with(:factdest).returns "fdest"
+        Puppet.settings.expects(:value).with(:factsignore).returns "fignore"
+
+        Puppet::Agent::Downloader.expects(:new).with("fact", "fsource", "fdest", "fignore").returns downloader
+
+        downloader.expects(:evaluate)
+
+        @facthandler.expects(:download_fact_plugins?).returns true
+        @facthandler.download_fact_plugins
+    end
+
+    it "should have a method for uploading facts" do
+        @facthandler.should respond_to(:upload_facts)
+    end
+
+    it "should reload Facter and find local facts when asked to upload facts" do
+        @facthandler.expects(:reload_facter)
+
+        Puppet.settings.expects(:value).with(:certname).returns "myhost"
+        Puppet::Node::Facts.expects(:find).with("myhost")
+
+        @facthandler.upload_facts
+    end
+
+    describe "when reloading Facter" do
+        before do
+            Facter.stubs(:clear)
+            Facter.stubs(:load)
+            Facter.stubs(:loadfacts)
+        end
+
+        it "should clear Facter" do
+            Facter.expects(:clear)
+            @facthandler.reload_facter
+        end
+
+        it "should load all Facter facts" do
+            Facter.expects(:loadfacts)
+            @facthandler.reload_facter
+        end
+
+        it "should load all Puppet Fact plugins" do
+            @facthandler.expects(:load_fact_plugins)
+            @facthandler.reload_facter
+        end
+    end
+
+    it "should load each directory in the Fact path when loading fact plugins" do
+        Puppet.settings.expects(:value).with(:factpath).returns("one%stwo" % File::PATH_SEPARATOR)
+
+        @facthandler.expects(:load_facts_in_dir).with("one")
+        @facthandler.expects(:load_facts_in_dir).with("two")
+
+        @facthandler.load_fact_plugins
+    end
+
+    it "should skip files when asked to load a directory" do
+        FileTest.expects(:directory?).with("myfile").returns false
+
+        @facthandler.load_facts_in_dir("myfile")
+    end
+
+    it "should load each ruby file when asked to load a directory" do
+        FileTest.expects(:directory?).with("mydir").returns true
+        Dir.expects(:chdir).with("mydir").yields
+
+        Dir.expects(:glob).with("*.rb").returns %w{a.rb b.rb}
+
+        @facthandler.expects(:load).with("a.rb")
+        @facthandler.expects(:load).with("b.rb")
+
+        @facthandler.load_facts_in_dir("mydir")
+    end
+end
diff --git a/spec/unit/agent/plugin_handler.rb b/spec/unit/agent/plugin_handler.rb
new file mode 100755
index 000000000..44603bc6c
--- /dev/null
+++ b/spec/unit/agent/plugin_handler.rb
@@ -0,0 +1,100 @@
+#!/usr/bin/env ruby
+
+require File.dirname(__FILE__) + '/../../spec_helper'
+require 'puppet/agent'
+require 'puppet/agent/plugin_handler'
+
+class PluginHandlerTester
+    include Puppet::Agent::PluginHandler
+end
+
+describe Puppet::Agent::PluginHandler do
+    before do
+        @pluginhandler = PluginHandlerTester.new
+    end
+
+    it "should have a method for downloading plugins" do
+        @pluginhandler.should respond_to(:download_plugins)
+    end
+
+    it "should have a boolean method for determining whether plugins should be downloaded" do
+        @pluginhandler.should respond_to(:download_plugins?)
+    end
+
+    it "should download plugins when :pluginsync is true" do
+        Puppet.settings.expects(:value).with(:pluginsync).returns true
+        @pluginhandler.should be_download_plugins
+    end
+
+    it "should not download plugins when :pluginsync is false" do
+        Puppet.settings.expects(:value).with(:pluginsync).returns false
+        @pluginhandler.should_not be_download_plugins
+    end
+
+    it "should not download plugins when downloading is disabled" do
+        Puppet::Agent::Downloader.expects(:new).never
+        @pluginhandler.expects(:download_plugins?).returns false
+        @pluginhandler.download_plugins
+    end
+
+    it "should use an Agent Downloader, with the name, source, destination, and ignore set correctly, to download plugins when downloading is enabled" do
+        downloader = mock 'downloader'
+
+        Puppet.settings.expects(:value).with(:pluginsource).returns "psource"
+        Puppet.settings.expects(:value).with(:plugindest).returns "pdest"
+        Puppet.settings.expects(:value).with(:pluginsignore).returns "pignore"
+
+        Puppet::Agent::Downloader.expects(:new).with("plugin", "psource", "pdest", "pignore").returns downloader
+
+        downloader.expects(:evaluate).returns []
+
+        @pluginhandler.expects(:download_plugins?).returns true
+        @pluginhandler.download_plugins
+    end
+
+    it "should be able to load plugins" do
+        @pluginhandler.should respond_to(:load_plugin)
+    end
+
+    it "should load each downloaded file" do
+        downloader = mock 'downloader'
+
+        Puppet::Agent::Downloader.expects(:new).returns downloader
+
+        downloader.expects(:evaluate).returns %w{one two}
+
+        @pluginhandler.expects(:download_plugins?).returns true
+
+        @pluginhandler.expects(:load_plugin).with("one")
+        @pluginhandler.expects(:load_plugin).with("two")
+
+        @pluginhandler.download_plugins
+    end
+
+    it "should load plugins when asked to do so" do
+        @pluginhandler.expects(:load).with("foo")
+
+        @pluginhandler.load_plugin("foo")
+    end
+
+    it "should not try to load directories" do
+        FileTest.expects(:directory?).with("foo").returns true
+        @pluginhandler.expects(:load).never
+
+        @pluginhandler.load_plugin("foo")
+    end
+
+    it "should warn but not fail if loading a file raises an exception" do
+        @pluginhandler.expects(:load).with("foo").raises "eh"
+
+        Puppet.expects(:err)
+        @pluginhandler.load_plugin("foo")
+    end
+
+    it "should warn but not fail if loading a file raises a LoadError" do
+        @pluginhandler.expects(:load).with("foo").raises LoadError.new("eh")
+
+        Puppet.expects(:err)
+        @pluginhandler.load_plugin("foo")
+    end
+end