Fixing #2336 - qualified variables only throw warnings

We were previously throwing exceptions.

This also ports all of the tests for variable lookup
over to rspec.

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

3 files changed, 106 insertions, 56 deletions

diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 0a182fe1d..bf34d2e29 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -174,10 +174,12 @@ class Puppet::Parser::Scope
         klassname = parts.join("::")
         klass = findclass(klassname)
         unless klass
-            raise Puppet::ParseError, "Could not find class %s" % klassname
+            warning "Could not look up qualified variable '%s'; class %s could not be found" % [name, klassname]
+            return usestring ? "" : :undefined
         end
         unless kscope = compiler.class_scope(klass)
-            raise Puppet::ParseError, "Class %s has not been evaluated so its variables cannot be referenced" % klass.classname
+            warning "Could not look up qualified variable '%s'; class %s has not been evaluated" % [name, klassname]
+            return usestring ? "" : :undefined
         end
         return kscope.lookupvar(shortname, usestring)
     end
diff --git a/spec/unit/parser/scope.rb b/spec/unit/parser/scope.rb
index 13559528b..b0c0a8ef3 100755
--- a/spec/unit/parser/scope.rb
+++ b/spec/unit/parser/scope.rb
@@ -4,13 +4,112 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 
 describe Puppet::Parser::Scope do
     before :each do
-        @scope = Puppet::Parser::Scope.new()
         @topscope = Puppet::Parser::Scope.new()
-        @scope.stubs(:parent).returns(@topscope)
+        # This is necessary so we don't try to use the compiler to discover our parent.
+        @topscope.parent = nil
+        @scope = Puppet::Parser::Scope.new()
+        @scope.parent = @topscope
     end
 
-    describe Puppet::Parser::Scope, "when setvar is called with append=true" do
+    describe "when looking up a variable" do
+        it "should default to an empty string" do
+            @scope.lookupvar("var").should == ""
+        end
+        
+        it "should return an string when asked for a string" do
+            @scope.lookupvar("var", true).should == ""
+        end
+
+        it "should return ':undefined' for unset variables when asked not to return a string" do
+            @scope.lookupvar("var", false).should == :undefined
+        end
+
+        it "should be able to look up values" do
+            @scope.setvar("var", "yep")
+            @scope.lookupvar("var").should == "yep"
+        end
+
+        it "should be able to look up variables in parent scopes" do
+            @topscope.setvar("var", "parentval")
+            @scope.lookupvar("var").should == "parentval"
+        end
+
+        it "should prefer its own values to parent values" do
+            @topscope.setvar("var", "parentval")
+            @scope.setvar("var", "childval")
+            @scope.lookupvar("var").should == "childval"
+        end
+
+        describe "and the variable is qualified" do
+            before do
+                @parser = Puppet::Parser::Parser.new()
+                @compiler = Puppet::Parser::Compiler.new(stub("node", :name => "foonode"), @parser)
+                @scope.compiler = @compiler
+                @scope.parser = @parser
+            end
+
+            def create_class_scope(name)
+                klass = @parser.newclass(name)
+                Puppet::Parser::Resource.new(:type => "class", :title => name, :scope => @scope, :source => mock('source')).evaluate
+
+                return @compiler.class_scope(klass)
+            end
+
+            it "should be able to look up explicitly fully qualified variables from main" do
+                other_scope = create_class_scope("")
+
+                other_scope.setvar("othervar", "otherval")
 
+                @scope.lookupvar("::othervar").should == "otherval"
+            end
+
+            it "should be able to look up explicitly fully qualified variables from other scopes" do
+                other_scope = create_class_scope("other")
+
+                other_scope.setvar("var", "otherval")
+
+                @scope.lookupvar("::other::var").should == "otherval"
+            end
+
+            it "should be able to look up deeply qualified variables" do
+                other_scope = create_class_scope("other::deep::klass")
+
+                other_scope.setvar("var", "otherval")
+
+                @scope.lookupvar("other::deep::klass::var").should == "otherval"
+            end
+
+            it "should return an empty string for qualified variables that cannot be found in other classes" do
+                other_scope = create_class_scope("other::deep::klass")
+
+                @scope.lookupvar("other::deep::klass::var").should == ""
+            end
+
+            it "should warn and return an empty string for qualified variables whose classes have not been evaluated" do
+                klass = @parser.newclass("other::deep::klass")
+                @scope.expects(:warning)
+                @scope.lookupvar("other::deep::klass::var").should == ""
+            end
+
+            it "should warn and return an empty string for qualified variables whose classes do not exist" do
+                @scope.expects(:warning)
+                @scope.lookupvar("other::deep::klass::var").should == ""
+            end
+
+            it "should return ':undefined' when asked for a non-string qualified variable from a class that does not exist" do
+                @scope.stubs(:warning)
+                @scope.lookupvar("other::deep::klass::var", false).should == :undefined
+            end
+
+            it "should return ':undefined' when asked for a non-string qualified variable from a class that has not been evaluated" do
+                @scope.stubs(:warning)
+                klass = @parser.newclass("other::deep::klass")
+                @scope.lookupvar("other::deep::klass::var", false).should == :undefined
+            end
+        end
+    end
+
+    describe Puppet::Parser::Scope, "when setvar is called with append=true" do
         it "should raise error if the variable is already defined in this scope" do
             @scope.setvar("var","1",nil,nil,false)
             lambda { @scope.setvar("var","1",nil,nil,true) }.should raise_error(Puppet::ParseError)
@@ -97,5 +196,4 @@ describe Puppet::Parser::Scope do
             Puppet::Parser::Scope.number?("0x89g").should be_nil
         end
     end
-
 end
diff --git a/test/language/scope.rb b/test/language/scope.rb
index debef44bf..403e569f7 100755
--- a/test/language/scope.rb
+++ b/test/language/scope.rb
@@ -83,56 +83,6 @@ class TestScope < Test::Unit::TestCase
                      "botscope values shadow parent scope values")
     end
 
-    def test_lookupvar
-        parser = mkparser
-        scope = mkscope :parser => parser
-
-        # first do the plain lookups
-        assert_equal("", scope.lookupvar("var"), "scope did not default to string")
-        assert_equal("", scope.lookupvar("var", true), "scope ignored usestring setting")
-        assert_equal(:undefined, scope.lookupvar("var", false), "scope ignored usestring setting when false")
-
-        # Now set the var
-        scope.setvar("var", "yep")
-        assert_equal("yep", scope.lookupvar("var"), "did not retrieve value correctly")
-
-        # Now test the parent lookups
-        subscope = mkscope :parser => parser
-        subscope.parent = scope
-        assert_equal("", subscope.lookupvar("nope"), "scope did not default to string with parent")
-        assert_equal("", subscope.lookupvar("nope", true), "scope ignored usestring setting with parent")
-        assert_equal(:undefined, subscope.lookupvar("nope", false), "scope ignored usestring setting when false with parent")
-
-        assert_equal("yep", subscope.lookupvar("var"), "did not retrieve value correctly from parent")
-
-        # Now override the value in the subscope
-        subscope.setvar("var", "sub")
-        assert_equal("sub", subscope.lookupvar("var"), "did not retrieve overridden value correctly")
-
-        # Make sure we punt when the var is qualified.  Specify the usestring value, so we know it propagates.
-        scope.expects(:lookup_qualified_var).with("one::two", false).returns(:punted)
-        assert_equal(:punted, scope.lookupvar("one::two", false), "did not return the value of lookup_qualified_var")
-    end
-
-    def test_lookup_qualified_var
-        parser = mkparser
-        scope = mkscope :parser => parser
-
-        scopes = {}
-        classes = ["", "one", "one::two", "one::two::three"].each do |name|
-            klass = parser.newclass(name)
-            Puppet::Parser::Resource.new(:type => "class", :title => name, :scope => scope, :source => mock('source')).evaluate
-            scopes[name] = scope.compiler.class_scope(klass)
-        end
-
-        classes.each do |name|
-            var = [name, "var"].join("::")
-            scopes[name].expects(:lookupvar).with("var", false).returns(name)
-
-            assert_equal(name, scope.send(:lookup_qualified_var, var, false), "did not get correct value from lookupvar")
-        end
-    end
-
     def test_declarative
         # set to declarative
         top = mkscope