From: Markus Roberts <mar...@reality.com> This patch adds an options hash to lookupvar analogous to the one taken by setvar and uses it to pass in source location for error reporting. It also fixes the mechanism used by setvar (file was not being passed correctly), adds line and file information to errors in templates, and extends/corrects tests.
As presently written it does not gather userful line numbers from inline templates and there are no tests for the template line number generation. Signed-off-by: Nick Lewis <n...@puppetlabs.com> --- Local-branch: ticket/next/5027 lib/puppet/parser/ast/leaf.rb | 4 +- lib/puppet/parser/ast/vardef.rb | 2 +- lib/puppet/parser/scope.rb | 9 +++-- lib/puppet/parser/templatewrapper.rb | 12 +++++-- spec/unit/parser/ast/arithmetic_operator_spec.rb | 10 ----- spec/unit/parser/ast/comparison_operator_spec.rb | 4 +- spec/unit/parser/ast/leaf_spec.rb | 25 ++++++++----- spec/unit/parser/ast/vardef_spec.rb | 19 +++++++--- spec/unit/parser/templatewrapper_spec.rb | 40 ++++++++++----------- 9 files changed, 66 insertions(+), 59 deletions(-) diff --git a/lib/puppet/parser/ast/leaf.rb b/lib/puppet/parser/ast/leaf.rb index b61634d..c8ebc94 100644 --- a/lib/puppet/parser/ast/leaf.rb +++ b/lib/puppet/parser/ast/leaf.rb @@ -124,7 +124,7 @@ class Puppet::Parser::AST # not include syntactical constructs, like '$' and '{}'). def evaluate(scope) parsewrap do - if (var = scope.lookupvar(@value)) == :undefined + if (var = scope.lookupvar(@value, :file => file, :line => line)) == :undefined var = :undef end var @@ -141,7 +141,7 @@ class Puppet::Parser::AST def evaluate_container(scope) container = variable.respond_to?(:evaluate) ? variable.safeevaluate(scope) : variable - (container.is_a?(Hash) or container.is_a?(Array)) ? container : scope.lookupvar(container) + (container.is_a?(Hash) or container.is_a?(Array)) ? container : scope.lookupvar(container, :file => file, :line => line) end def evaluate_key(scope) diff --git a/lib/puppet/parser/ast/vardef.rb b/lib/puppet/parser/ast/vardef.rb index 6de1860..b766311 100644 --- a/lib/puppet/parser/ast/vardef.rb +++ b/lib/puppet/parser/ast/vardef.rb @@ -20,7 +20,7 @@ class Puppet::Parser::AST name = @name.safeevaluate(scope) parsewrap do - scope.setvar(name,value, :file => @file, :line => @line, :append => @append) + scope.setvar(name,value, :file => file, :line => line, :append => @append) end end end diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb index 95b765b..a61a857 100644 --- a/lib/puppet/parser/scope.rb +++ b/lib/puppet/parser/scope.rb @@ -223,21 +223,22 @@ class Puppet::Parser::Scope private :qualified_scope # Look up a variable. The simplest value search we do. - def lookupvar(name) + def lookupvar(name, options = {}) table = ephemeral?(name) ? @ephemeral.last : @symtable # If the variable is qualified, then find the specified scope and look the variable up there instead. if name =~ /^(.*)::(.+)$/ begin - qualified_scope($1).lookupvar($2) + qualified_scope($1).lookupvar($2,options) rescue RuntimeError => e - warning "Could not look up qualified variable '#{name}'; #{e.message}" + location = (options[:file] && options[:line]) ? " at #{options[:file]}:#{options[:line]}" : '' + warning "Could not look up qualified variable '#{name}'; #{e.message}#{location}" :undefined end elsif ephemeral_include?(name) or table.include?(name) # We can't use "if table[name]" here because the value might be false table[name] elsif parent - parent.lookupvar(name) + parent.lookupvar(name,options) else :undefined end diff --git a/lib/puppet/parser/templatewrapper.rb b/lib/puppet/parser/templatewrapper.rb index 73fcb8a..180a03d 100644 --- a/lib/puppet/parser/templatewrapper.rb +++ b/lib/puppet/parser/templatewrapper.rb @@ -18,9 +18,14 @@ class Puppet::Parser::TemplateWrapper @__scope__ end + def script_line + # find which line in the template (if any) we were called from + caller.find { |l| l =~ /#{file}:/ }.first[/:(\d+):/,1] + end + # Should return true if a variable is defined, false if it is not def has_variable?(name) - scope.lookupvar(name.to_s) != :undefined + scope.lookupvar(name.to_s, :file => file, :line => script_line) != :undefined end # Allow templates to access the defined classes @@ -51,13 +56,13 @@ class Puppet::Parser::TemplateWrapper # the missing_method definition here until we declare the syntax finally # dead. def method_missing(name, *args) - value = scope.lookupvar(name.to_s) + value = scope.lookupvar(name.to_s,:file => file,:line => script_line) if value != :undefined return value else # Just throw an error immediately, instead of searching for # other missingmethod things or whatever. - raise Puppet::ParseError, "Could not find value for '#{name}'" + raise Puppet::ParseError.new("Could not find value for '#{name}'",@file,script_line) end end @@ -97,6 +102,7 @@ class Puppet::Parser::TemplateWrapper result = nil benchmark(:debug, "Interpolated template #{template_source}") do template = ERB.new(self.string, 0, "-") + template.filename = file result = template.result(binding) end diff --git a/spec/unit/parser/ast/arithmetic_operator_spec.rb b/spec/unit/parser/ast/arithmetic_operator_spec.rb index 0aab6f0..9b54f09 100755 --- a/spec/unit/parser/ast/arithmetic_operator_spec.rb +++ b/spec/unit/parser/ast/arithmetic_operator_spec.rb @@ -60,14 +60,4 @@ describe Puppet::Parser::AST::ArithmeticOperator do operator.evaluate(@scope).should == 4.33 end - it "should work for variables too" do - @scope.expects(:lookupvar).with("one").returns(1) - @scope.expects(:lookupvar).with("two").returns(2) - one = ast::Variable.new( :value => "one" ) - two = ast::Variable.new( :value => "two" ) - - operator = ast::ArithmeticOperator.new :lval => one, :operator => "+", :rval => two - operator.evaluate(@scope).should == 3 - end - end diff --git a/spec/unit/parser/ast/comparison_operator_spec.rb b/spec/unit/parser/ast/comparison_operator_spec.rb index 3efe28b..0608273 100755 --- a/spec/unit/parser/ast/comparison_operator_spec.rb +++ b/spec/unit/parser/ast/comparison_operator_spec.rb @@ -95,8 +95,8 @@ describe Puppet::Parser::AST::ComparisonOperator do one = Puppet::Parser::AST::Variable.new( :value => "one" ) two = Puppet::Parser::AST::Variable.new( :value => "two" ) - @scope.expects(:lookupvar).with("one").returns(1) - @scope.expects(:lookupvar).with("two").returns(2) + one.expects(:safeevaluate).with(@scope).returns(1) + two.expects(:safeevaluate).with(@scope).returns(2) operator = Puppet::Parser::AST::ComparisonOperator.new :lval => one, :operator => "<", :rval => two operator.evaluate(@scope).should == true diff --git a/spec/unit/parser/ast/leaf_spec.rb b/spec/unit/parser/ast/leaf_spec.rb index 97c996b..64fdcd6 100755 --- a/spec/unit/parser/ast/leaf_spec.rb +++ b/spec/unit/parser/ast/leaf_spec.rb @@ -107,7 +107,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do describe "when evaluating" do it "should evaluate the variable part if necessary" do - @scope.stubs(:lookupvar).with("a").returns(["b"]) + @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns(["b"]) variable = stub 'variable', :evaluate => "a" access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => variable, :key => 0 ) @@ -118,7 +118,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do end it "should evaluate the access key part if necessary" do - @scope.stubs(:lookupvar).with("a").returns(["b"]) + @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns(["b"]) index = stub 'index', :evaluate => 0 access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => index ) @@ -129,7 +129,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do end it "should be able to return an array member" do - @scope.stubs(:lookupvar).with("a").returns(["val1", "val2", "val3"]) + @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns(["val1", "val2", "val3"]) access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => 1 ) @@ -153,7 +153,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do end it "should be able to return an hash value" do - @scope.stubs(:lookupvar).with("a").returns({ "key1" => "val1", "key2" => "val2", "key3" => "val3" }) + @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns({ "key1" => "val1", "key2" => "val2", "key3" => "val3" }) access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key2" ) @@ -177,7 +177,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do end it "should raise an error if the variable wasn't in the scope" do - @scope.stubs(:lookupvar).with("a").returns(nil) + @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns(nil) access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key2" ) @@ -190,7 +190,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do end it "should work with recursive hash access" do - @scope.stubs(:lookupvar).with("a").returns({ "key" => { "subkey" => "b" }}) + @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns({ "key" => { "subkey" => "b" }}) access1 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key") access2 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => access1, :key => "subkey") @@ -199,7 +199,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do end it "should work with interleaved array and hash access" do - @scope.stubs(:lookupvar).with("a").returns({ "key" => [ "a" , "b" ]}) + @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns({ "key" => [ "a" , "b" ]}) access1 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key") access2 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => access1, :key => 1) @@ -332,16 +332,21 @@ end describe Puppet::Parser::AST::Variable do before :each do @scope = stub 'scope' - @var = Puppet::Parser::AST::Variable.new(:value => "myvar") + @var = Puppet::Parser::AST::Variable.new(:value => "myvar", :file => 'my.pp', :line => 222) end it "should lookup the variable in scope" do - @scope.expects(:lookupvar).with("myvar").returns(:myvalue) + @scope.expects(:lookupvar).with { |name,options| name == "myvar" }.returns(:myvalue) + @var.safeevaluate(@scope).should == :myvalue + end + + it "should pass the source location to lookupvar" do + @scope.expects(:lookupvar).with { |name,options| name == "myvar" and options[:file] == 'my.pp' and options[:line] == 222 }.returns(:myvalue) @var.safeevaluate(@scope).should == :myvalue end it "should return undef if the variable wasn't set" do - @scope.expects(:lookupvar).with("myvar").returns(:undefined) + @scope.expects(:lookupvar).with { |name,options| name == "myvar" }.returns(:undefined) @var.safeevaluate(@scope).should == :undef end diff --git a/spec/unit/parser/ast/vardef_spec.rb b/spec/unit/parser/ast/vardef_spec.rb index a462e8e..5a851bb 100755 --- a/spec/unit/parser/ast/vardef_spec.rb +++ b/spec/unit/parser/ast/vardef_spec.rb @@ -16,8 +16,7 @@ describe Puppet::Parser::AST::VarDef do name.expects(:safeevaluate).with(@scope) value.expects(:safeevaluate).with(@scope) - vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, - :line => nil + vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, :line => nil vardef.evaluate(@scope) end @@ -27,8 +26,7 @@ describe Puppet::Parser::AST::VarDef do @scope.expects(:setvar).with { |name,value,options| options[:append] == nil } - vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, - :line => nil + vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, :line => nil vardef.evaluate(@scope) end @@ -38,8 +36,17 @@ describe Puppet::Parser::AST::VarDef do @scope.expects(:setvar).with { |name,value,options| options[:append] == true } - vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, - :line => nil, :append => true + vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, :line => nil, :append => true + vardef.evaluate(@scope) + end + + it "should call pass the source location to setvar" do + name = stub 'name', :safeevaluate => "var" + value = stub 'value', :safeevaluate => "1" + + @scope.expects(:setvar).with { |name,value,options| options[:file] == 'setvar.pp' and options[:line] == 917 } + + vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => 'setvar.pp', :line => 917 vardef.evaluate(@scope) end diff --git a/spec/unit/parser/templatewrapper_spec.rb b/spec/unit/parser/templatewrapper_spec.rb index 4a713b8..0c7199b 100755 --- a/spec/unit/parser/templatewrapper_spec.rb +++ b/spec/unit/parser/templatewrapper_spec.rb @@ -17,6 +17,12 @@ describe Puppet::Parser::TemplateWrapper do @tw = Puppet::Parser::TemplateWrapper.new(@scope) end + def mock_template(source=nil) + template_mock = mock("template", :result => "woot!") + ERB.expects(:new).with("template contents", 0, "-").returns(template_mock) + template_mock.expects(:filename=).with(source) + end + it "should create a new object TemplateWrapper from a scope" do tw = Puppet::Parser::TemplateWrapper.new(@scope) @@ -54,41 +60,38 @@ describe Puppet::Parser::TemplateWrapper do end it "should return the processed template contents with a call to result" do - template_mock = mock("template", :result => "woot!") + mock_template("/tmp/fake_template") File.expects(:read).with("/tmp/fake_template").returns("template contents") - ERB.expects(:new).with("template contents", 0, "-").returns(template_mock) @tw.file = @file @tw.result.should eql("woot!") end it "should return the processed template contents with a call to result and a string" do - template_mock = mock("template", :result => "woot!") - ERB.expects(:new).with("template contents", 0, "-").returns(template_mock) - + mock_template @tw.result("template contents").should eql("woot!") end it "should return the contents of a variable if called via method_missing" do - @scope.expects(:lookupvar).with("chicken").returns("is good") + @scope.expects(:lookupvar).with { |name,options| name == "chicken"}.returns("is good") tw = Puppet::Parser::TemplateWrapper.new(@scope) tw.chicken.should eql("is good") end it "should throw an exception if a variable is called via method_missing and it does not exist" do - @scope.expects(:lookupvar).with("chicken").returns(:undefined) + @scope.expects(:lookupvar).with { |name,options| name == "chicken"}.returns(:undefined) tw = Puppet::Parser::TemplateWrapper.new(@scope) lambda { tw.chicken }.should raise_error(Puppet::ParseError) end it "should allow you to check whether a variable is defined with has_variable?" do - @scope.expects(:lookupvar).with("chicken").returns("is good") + @scope.expects(:lookupvar).with { |name,options| name == "chicken"}.returns("is good") tw = Puppet::Parser::TemplateWrapper.new(@scope) tw.has_variable?("chicken").should eql(true) end it "should allow you to check whether a variable is not defined with has_variable?" do - @scope.expects(:lookupvar).with("chicken").returns(:undefined) + @scope.expects(:lookupvar).with { |name,options| name == "chicken"}.returns(:undefined) tw = Puppet::Parser::TemplateWrapper.new(@scope) tw.has_variable?("chicken").should eql(false) end @@ -114,9 +117,7 @@ describe Puppet::Parser::TemplateWrapper do end it "should set all of the scope's variables as instance variables" do - template_mock = mock("template", :result => "woot!") - ERB.expects(:new).with("template contents", 0, "-").returns(template_mock) - + mock_template @scope.expects(:to_hash).returns("one" => "foo") @tw.result("template contents") @@ -124,8 +125,7 @@ describe Puppet::Parser::TemplateWrapper do end it "should not error out if one of the variables is a symbol" do - template_mock = mock("template", :result => "woot!") - ERB.expects(:new).with("template contents", 0, "-").returns(template_mock) + mock_template @scope.expects(:to_hash).returns(:_timestamp => "1234") @tw.result("template contents") @@ -133,13 +133,11 @@ describe Puppet::Parser::TemplateWrapper do %w{! . ; :}.each do |badchar| it "should translate #{badchar} to _ when setting the instance variables" do - template_mock = mock("template", :result => "woot!") - ERB.expects(:new).with("template contents", 0, "-").returns(template_mock) - - @scope.expects(:to_hash).returns("one#{badchar}" => "foo") - @tw.result("template contents") + mock_template + @scope.expects(:to_hash).returns("one#{badchar}" => "foo") + @tw.result("template contents") - @tw.instance_variable_get("@one_").should == "foo" - end + @tw.instance_variable_get("@one_").should == "foo" + end end end -- 1.7.4.1 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to puppet-dev@googlegroups.com. To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.