[Pkg-puppet-devel] [facter] 154/180: (maint) Avoid repeated fact evaluation against different loaders

[Stig Sandbeck Mathisen]

This is an automated email from the git hooks/post-receive script.

ssm pushed a commit to branch master
in repository facter.

commit ded4090cbe3a4cbc8a11660ec13c561c37f06f42
Author: Josh Cooper <josh at puppetlabs.com>
Date:   Thu Jun 19 16:49:41 2014 -0700

    (maint) Avoid repeated fact evaluation against different loaders
    
    Commit 4d937456e fixed an infinite recursion problem where Facter could load
    a fact, e.g. ec2, and outside of the setcode body, the fact could reference
    another fact, e.g. macaddress, that had not yet been evaluated. This results
    in a fact cache miss, causing Facter to reload all facts, including the
    original one, e.g. ec2, leading to an infinite loop.
    
    The spec test was written to load nosuchfact.rb, which contained ruby
    code that tried to resolve :nosuchfact, causing a cache miss, and
    to ensure we didn't infinitely recurse.
    
    However, the spec test did not account that there are two loaders in play,
    so if you run:
    
        $ bundle exec rspec spec/unit/util/loader_spec.rb
        Already evaluated rest at /Users/josh/work/facter/lib/facter/ec2.rb:4, reevaluating anyways
        Already evaluated rest at /Users/josh/work/facter/lib/facter/ec2.rb:21, reevaluating anyways
    
    The reason is because the spec test creates a loader that defines the
    ec2 fact and its rest resolution. However, when nosuchfact.rb is loaded,
    it calls Facter.value, which delegates to a different loader than the one the
    test created. As a result, the ec2 fact is evaluated twice.
    
    This commit simplifies the spec test to ensure that Kernel.load is only
    called once for a given fact.
---
 lib/facter/util/loader.rb                    | 11 ++++++++++-
 spec/fixtures/unit/util/loader/nosuchfact.rb |  1 -
 spec/unit/util/loader_spec.rb                |  8 ++++----
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/lib/facter/util/loader.rb b/lib/facter/util/loader.rb
index 0f31bd5..37dd177 100644
--- a/lib/facter/util/loader.rb
+++ b/lib/facter/util/loader.rb
@@ -112,7 +112,7 @@ class Facter::Util::Loader
     begin
       # Store the file path so we don't try to reload it
       @loaded << file
-      Kernel.load(file)
+      kernel_load(file)
     rescue ScriptError => detail
       # Don't store the path if the file can't be loaded
       # in case it's loadable later on.
@@ -121,6 +121,15 @@ class Facter::Util::Loader
     end
   end
 
+  # Load and execute the Ruby program specified in the file. This exists
+  # for testing purposes.
+  #
+  # @api private
+  # @return [Boolean]
+  def kernel_load(file)
+    Kernel.load(file)
+  end
+
   # Load facts from the environment.  If no name is provided,
   # all will be loaded.
   def load_env(fact = nil)
diff --git a/spec/fixtures/unit/util/loader/nosuchfact.rb b/spec/fixtures/unit/util/loader/nosuchfact.rb
deleted file mode 100644
index f0ba60a..0000000
--- a/spec/fixtures/unit/util/loader/nosuchfact.rb
+++ /dev/null
@@ -1 +0,0 @@
-Facter.value(:nosuchfact)
diff --git a/spec/unit/util/loader_spec.rb b/spec/unit/util/loader_spec.rb
index 667d96c..b87a196 100755
--- a/spec/unit/util/loader_spec.rb
+++ b/spec/unit/util/loader_spec.rb
@@ -277,10 +277,10 @@ describe Facter::Util::Loader do
   end
 
   it "should load facts on the facter search path only once" do
-    facterlibdir = File.expand_path(File.dirname(__FILE__) + '../../../fixtures/unit/util/loader')
+    loader = loader_from(:env => {})
+    loader.load_all
 
-    Facter::Util::Loader.new('FACTERLIB' => facterlibdir).load_all
-
-    Facter.value(:nosuchfact).should be_nil
+    loader.expects(:kernel_load).with(regexp_matches(/ec2/)).never
+    loader.load(:ec2)
   end
 end

-- 
Alioth's /usr/local/bin/git-commit-notice on /srv/git.debian.org/git/pkg-puppet/facter.git