On Jan 24, 2011, at 4:46 PM, Matt Robinson wrote:
> Renamed some variables to be clearer, made tests use less stubbing,
> added some additional tests and got rid of some unecessary logic.
>
> Paired-with: Dan Bode
>
> Signed-off-by: Matt Robinson <[email protected]>
> ---
> lib/puppet/parser/compiler.rb | 2 +-
> lib/puppet/resource/type.rb | 12 +++---
> spec/unit/parser/compiler_spec.rb | 84 +++++++++++++++++++++++++------------
> spec/unit/resource/type_spec.rb | 11 ++---
> 4 files changed, 69 insertions(+), 40 deletions(-)
>
> diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/compiler.rb
> index 3134e03..fdabd05 100644
> --- a/lib/puppet/parser/compiler.rb
> +++ b/lib/puppet/parser/compiler.rb
> @@ -153,7 +153,7 @@ class Puppet::Parser::Compiler
> if param_classes
> resource = klass.ensure_in_catalog(scope, param_classes[name] || {})
> else
> - found << name and next if scope.class_scope(klass) and
> !param_classes
> + found << name and next if scope.class_scope(klass)
> resource = klass.ensure_in_catalog(scope)
> end
>
> diff --git a/lib/puppet/resource/type.rb b/lib/puppet/resource/type.rb
> index a31795a..92e1ef7 100644
> --- a/lib/puppet/resource/type.rb
> +++ b/lib/puppet/resource/type.rb
> @@ -143,23 +143,23 @@ class Puppet::Resource::Type
> # classes and nodes. No parameters are be supplied--if this is a
> # parameterized class, then all parameters take on their default
> # values.
> - def ensure_in_catalog(scope, attributes=nil)
> + def ensure_in_catalog(scope, parameters=nil)
> type == :definition and raise ArgumentError, "Cannot create resources for
> defined resource types"
> resource_type = type == :hostclass ? :class : :node
>
> # Do nothing if the resource already exists; this makes sure we don't
> # get multiple copies of the class resource, which helps provide the
> # singleton nature of classes.
> - # we should not do this for classes with attributes
> - # if attributes are passed, we should still try to create the resource
> + # we should not do this for classes with parameters
> + # if parameters are passed, we should still try to create the resource
> # even if it exists so that we can fail
> # this prevents us from being able to combine param classes with include
> - if resource = scope.catalog.resource(resource_type, name) and !attributes
> + if resource = scope.catalog.resource(resource_type, name) and !parameters
> return resource
> end
> resource = Puppet::Parser::Resource.new(resource_type, name, :scope =>
> scope, :source => self)
> - if attributes
> - attributes.each do |k,v|
> + if parameters
> + parameters.each do |k,v|
> resource.set_parameter(k,v)
> end
> end
> diff --git a/spec/unit/parser/compiler_spec.rb
> b/spec/unit/parser/compiler_spec.rb
> index c1e7c48..687f2ec 100755
> --- a/spec/unit/parser/compiler_spec.rb
> +++ b/spec/unit/parser/compiler_spec.rb
> @@ -105,8 +105,7 @@ describe Puppet::Parser::Compiler do
> node.classes = %w{foo bar}
> compiler = Puppet::Parser::Compiler.new(node)
>
> - compiler.classlist.should include("foo")
> - compiler.classlist.should include("bar")
> + compiler.classlist.should =~ ['foo', 'bar']
> end
>
> it "should transform node class hashes into a class list" do
> @@ -114,8 +113,7 @@ describe Puppet::Parser::Compiler do
> node.classes = {'foo'=>{'one'=>'1'}, 'bar'=>{'two'=>'2'}}
> compiler = Puppet::Parser::Compiler.new(node)
>
> - compiler.classlist.should include("foo")
> - compiler.classlist.should include("bar")
> + compiler.classlist.should =~ ['foo', 'bar']
> end
>
> it "should add a 'main' stage to the catalog" do
> @@ -549,7 +547,7 @@ describe Puppet::Parser::Compiler do
>
> @compiler.add_collection(coll)
>
> - lambda { @compiler.compile }.should raise_error(Puppet::ParseError)
> + lambda { @compiler.compile }.should raise_error Puppet::ParseError,
> 'Failed to realize virtual resources something'
I know most people don't run with warnings enabled, but I am nearly positive
this throws a warning (along with similar lines below), and I think the warning
says something about 1.9 compatibility. Throwing some extra parens in here
would fix it.
> end
>
> it "should fail when there are unevaluated resource collections that
> refer to multiple specific resources" do
> @@ -558,7 +556,7 @@ describe Puppet::Parser::Compiler do
>
> @compiler.add_collection(coll)
>
> - lambda { @compiler.compile }.should raise_error(Puppet::ParseError)
> + lambda { @compiler.compile }.should raise_error Puppet::ParseError,
> 'Failed to realize virtual resources one, two'
> end
> end
>
> @@ -611,28 +609,68 @@ describe Puppet::Parser::Compiler do
> @compiler.evaluate_classes(%w{myclass}, @scope)
> end
>
> - # test that ensure_in_catalog is called for existing classes
> - it "should ensure each node class hash is in catalog" do
> - klasses = {'foo'=>{'1'=>'one'}, 'bar'=>{'2'=>'two'}}
> + it "should ensure each node class hash is in catalog and have
> appropriate parameters" do
> + klasses = {'foo'=>{'1'=>'one'}, 'bar::foo'=>{'2'=>'two'},
> 'bar'=>{'1'=> [1,2,3], '2'=>{'foo'=>'bar'}}}
> @node.classes = klasses
> + ast_obj = Puppet::Parser::AST::String.new(:value => 'foo')
> klasses.each do |name, params|
> - klass = Puppet::Resource::Type.new(:hostclass, name)
> - @compiler.topscope.stubs(:find_hostclass).with(name).returns(klass)
> - klass.expects(:ensure_in_catalog).with(@compiler.topscope, params)
> + klass = Puppet::Resource::Type.new(:hostclass, name, :arguments =>
> {'1' => ast_obj, '2' => ast_obj})
> + @compiler.topscope.known_resource_types.add klass
> end
> - @compiler.compile
> + catalog = @compiler.compile
> + catalog.classes.should =~ ['foo', 'bar::foo', 'settings', 'bar']
> +
> + r1 = catalog.resources.detect {|r| r.title == 'Foo' }
> + r1.to_hash.should == {:'1' => 'one', :'2' => 'foo'}
> + r1.tags. should =~ ['class', 'foo']
> +
> + r2 = catalog.resources.detect {|r| r.title == 'Bar::Foo' }
> + r2.to_hash.should == {:'1' => 'foo', :'2' => 'two'}
> + r2.tags.should =~ ['bar::foo', 'class', 'bar', 'foo']
> +
> + r2 = catalog.resources.detect {|r| r.title == 'Bar' }
> + r2.to_hash.should == {:'1' => [1,2,3], :'2' => {'foo'=>'bar'}}
> + r2.tags.should =~ ['class', 'bar']
> + end
> +
> + it "should ensure each node class is in catalog and has appropriate
> tags" do
> + klasses = ['bar::foo']
> + @node.classes = klasses
> + ast_obj = Puppet::Parser::AST::String.new(:value => 'foo')
> + klasses.each do |name|
> + klass = Puppet::Resource::Type.new(:hostclass, name, :arguments =>
> {'1' => ast_obj, '2' => ast_obj})
> + @compiler.topscope.known_resource_types.add klass
> + end
> + catalog = @compiler.compile
> +
> + r2 = catalog.resources.detect {|r| r.title == 'Bar::Foo' }
> + r2.tags.should =~ ['bar::foo', 'class', 'bar', 'foo']
> + end
> +
> + it "should fail if required parameters are missing" do
> + klass = {'foo'=>{'1'=>'one'}}
> + @node.classes = klass
> + klass = Puppet::Resource::Type.new(:hostclass, 'foo', :arguments =>
> {'1' => nil, '2' => nil})
> + @compiler.topscope.known_resource_types.add klass
> + lambda { @compiler.compile }.should raise_error Puppet::ParseError,
> "Must pass 2 to Class[Foo]"
> + end
> +
> + it "should fail if invalid parameters are passed" do
> + klass = {'foo'=>{'3'=>'one'}}
> + @node.classes = klass
> + klass = Puppet::Resource::Type.new(:hostclass, 'foo', :arguments =>
> {'1' => nil, '2' => nil})
> + @compiler.topscope.known_resource_types.add klass
> + lambda { @compiler.compile }.should raise_error Puppet::ParseError,
> "Invalid parameter 3"
> end
>
> it "should ensure class is in catalog without params" do
> @node.classes = klasses = {'foo'=>nil}
> foo = Puppet::Resource::Type.new(:hostclass, 'foo')
> - @compiler.topscope.stubs(:find_hostclass).with('foo').returns(foo)
> - foo.expects(:ensure_in_catalog).with(@compiler.topscope, {})
> - @compiler.compile
> + @compiler.topscope.known_resource_types.add foo
> + catalog = @compiler.compile
> + catalog.classes.should include 'foo'
> end
>
> -
> -
> it "should not evaluate the resources created for found classes unless
> asked" do
> @compiler.catalog.stubs(:tag)
>
> @@ -667,14 +705,6 @@ describe Puppet::Parser::Compiler do
> @compiler.evaluate_classes(%w{myclass}, @scope, false)
> end
>
> - it "should not skip param classes that have already been evaluated" do
> - @scope.stubs(:class_scope).with(@class).returns("something")
> - @node.classes = klasses = {'foo'=>nil}
> - foo = Puppet::Resource::Type.new(:hostclass, 'foo')
> - @compiler.topscope.stubs(:find_hostclass).with('foo').returns(foo)
> - foo.expects(:ensure_in_catalog).with(@compiler.topscope, {})
> - @compiler.compile
> - end
> it "should skip classes previously evaluated with different
> capitalization" do
> @compiler.catalog.stubs(:tag)
> @scope.stubs(:find_hostclass).with("MyClass").returns(@class)
> @@ -814,7 +844,7 @@ describe Puppet::Parser::Compiler do
> it "should fail if the compile is finished and resource overrides have
> not been applied" do
> @compiler.add_override(@override)
>
> - lambda { @compiler.compile }.should raise_error(Puppet::ParseError)
> + lambda { @compiler.compile }.should raise_error Puppet::ParseError,
> 'Could not find resource(s) File[/foo] for overriding'
> end
> end
> end
> diff --git a/spec/unit/resource/type_spec.rb b/spec/unit/resource/type_spec.rb
> index 11288f1..206616e 100755
> --- a/spec/unit/resource/type_spec.rb
> +++ b/spec/unit/resource/type_spec.rb
> @@ -496,7 +496,7 @@ describe Puppet::Resource::Type do
>
> it "should evaluate the parent's resource" do
> @type.parent_type(@scope)
> -
> +
> @type.evaluate_code(@resource)
>
> @scope.class_scope(@parent_type).should_not be_nil
> @@ -504,7 +504,7 @@ describe Puppet::Resource::Type do
>
> it "should not evaluate the parent's resource if it has already been
> evaluated" do
> @parent_resource.evaluate
> -
> +
> @type.parent_type(@scope)
>
> @parent_resource.expects(:evaluate).never
> @@ -545,7 +545,7 @@ describe Puppet::Resource::Type do
>
> it "should not evaluate the parent's resource if it has already been
> evaluated" do
> @parent_resource.evaluate
> -
> +
> @type.parent_type(@scope)
>
> @parent_resource.expects(:evaluate).never
> @@ -575,7 +575,7 @@ describe Puppet::Resource::Type do
> @code = Puppet::Resource::TypeCollection.new("env")
> @code.add @top
> @code.add @middle
> -
> +
> @node.environment.stubs(:known_resource_types).returns(@code)
> end
>
> @@ -601,7 +601,7 @@ describe Puppet::Resource::Type do
> @compiler.catalog.resource(:class, "top").should
> be_instance_of(Puppet::Parser::Resource)
> end
>
> - it "should add specified attributes to the resource" do
> + it "should add specified parameters to the resource" do
> @top.ensure_in_catalog(@scope, {'one'=>'1', 'two'=>'2'})
> @compiler.catalog.resource(:class, "top")['one'].should == '1'
> @compiler.catalog.resource(:class, "top")['two'].should == '2'
> @@ -628,7 +628,6 @@ describe Puppet::Resource::Type do
> othertop = Puppet::Parser::Resource.new(:class, 'top',:source =>
> @source, :scope => @scope )
> # add the same class resource to the catalog
> @compiler.catalog.add_resource(othertop)
> - @compiler.catalog.expects(:resource).with(:class, 'top').returns true
> lambda { @top.ensure_in_catalog(@scope, {}) }.should
> raise_error(Puppet::Resource::Catalog::DuplicateResourceError)
> end
>
> --
> 1.7.3.1
>
> --
> You received this message because you are subscribed to the Google Groups
> "Puppet Developers" group.
> To post to this group, send email to [email protected].
> To unsubscribe from this group, send email to
> [email protected].
> For more options, visit this group at
> http://groups.google.com/group/puppet-dev?hl=en.
>
--
There is nothing so useless as doing efficiently that which should not
be done at all. -- Peter Drucker
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
