Fix 2455 - improve error handling on fact load

Fix facts added with empty blocks by handling calls to value when setcode not called

Ensure we handle load failures more gracefully

4 files changed, 37 insertions, 1 deletions

diff --git a/lib/facter/util/loader.rb b/lib/facter/util/loader.rb
index c6013cc..ac90c48 100644
--- a/lib/facter/util/loader.rb
+++ b/lib/facter/util/loader.rb
@@ -69,7 +69,11 @@ class Facter::Util::Loader
 
     def load_file(file)
         # We have to specify Kernel.load, because we have a load method.
-        Kernel.load(file)
+        begin
+            Kernel.load(file)
+        rescue ScriptError => detail
+            warn "Error loading fact #{file} #{detail}"
+        end
     end
 
     # Load facts from the environment.  If no name is provided,
diff --git a/lib/facter/util/resolution.rb b/lib/facter/util/resolution.rb
index 04d5e47..b9e28e8 100644
--- a/lib/facter/util/resolution.rb
+++ b/lib/facter/util/resolution.rb
@@ -111,6 +111,7 @@ class Facter::Util::Resolution
     # How we get a value for our resolution mechanism.
     def value
         result = nil
+        return result if @code == nil and @interpreter == nil
         begin
             Timeout.timeout(limit) do
                 if @code.is_a?(Proc)
diff --git a/spec/unit/util/loader.rb b/spec/unit/util/loader.rb
index 945d3af..0a28020 100755
--- a/spec/unit/util/loader.rb
+++ b/spec/unit/util/loader.rb
@@ -201,6 +201,16 @@ describe Facter::Util::Loader do
             @loader.load_all
         end
 
+        it "should not raise an exception when a file is unloadable" do
+            @loader.expects(:search_path).returns %w{/one/dir}
+            Dir.expects(:entries).with("/one/dir").returns %w{a.rb}
+
+            Kernel.expects(:load).with("/one/dir/a.rb").raises(LoadError)
+            @loader.expects(:warn)
+
+            lambda { @loader.load_all }.should_not raise_error
+        end
+
         it "should load all facts from the environment" do
             Facter.expects(:add).with('one')
             Facter.expects(:add).with('two')
diff --git a/spec/unit/util/resolution.rb b/spec/unit/util/resolution.rb
index 35581e5..d4bb781 100755
--- a/spec/unit/util/resolution.rb
+++ b/spec/unit/util/resolution.rb
@@ -25,6 +25,14 @@ describe Facter::Util::Resolution do
         Facter::Util::Resolution.new("yay").limit.should == 0
     end
 
+    it "should default to nil for code" do
+        Facter::Util::Resolution.new("yay").code.should be_nil
+    end
+
+    it "should default to nil for interpreter" do
+        Facter::Util::Resolution.new("yay").interpreter.should be_nil
+    end
+
     it "should provide a 'limit' method that returns the timeout" do
         res = Facter::Util::Resolution.new("yay")
         res.timeout = "testing"
@@ -71,6 +79,13 @@ describe Facter::Util::Resolution do
             @resolve = Facter::Util::Resolution.new("yay")
         end
 
+        describe "and setcode has not been called" do
+            it "should return nil" do
+                Facter::Util::Resolution.expects(:exec).with(nil, nil).never
+                @resolve.value.should be_nil
+            end
+        end
+
         describe "and the code is a string" do
             it "should return the result of executing the code with the interpreter" do
                 @resolve.setcode "/bin/foo"
@@ -103,11 +118,17 @@ describe Facter::Util::Resolution do
                 @resolve.value.should be_nil
             end
 
+            it "should return nil if the value is an empty block" do
+                @resolve.setcode { "" }
+                @resolve.value.should be_nil
+            end
+
             it "should use its limit method to determine the timeout, to avoid conflict when a 'timeout' method exists for some other reason" do
                 @resolve.expects(:timeout).never
                 @resolve.expects(:limit).returns "foo"
                 Timeout.expects(:timeout).with("foo")
 
+                @resolve.setcode { sleep 2; "raise This is a test"}
                 @resolve.value
             end