On Wed, Sep 22, 2010 at 2:53 PM, Jacob Helwig <[email protected]> wrote:
> > - private
> > -
> > - def mk_resource_type(type, name, options, code)
> > + def __mk_resource_type__(type, name, options, code)
> > klass = Puppet::Resource::Type.new(type, name, options)
>
> We already discussed this in person, but: There should be a comment here
> stating why "private" is insufficient protection on these methods.
>
Agreed.
> > + it "should allow node declarations" do
> > + write_file('foo.rb', ["node 'mynode' do notify('mynode') end",
> > + "node 'theirnode' do notify('theirnode')
> end"].join("\n"))
> > + catalog = compile("import 'foo'")
> > + catalog.resource("Notify[mynode]").should_not be_nil
> > + catalog.resource("Notify[theirnode]").should be_nil
> > + end
>
> The "should ..." text should probably be expanded to state that 'mynode'
> is the current node, and that is why it is not nil. Perhaps adding this
> to the describe, would be better ("Pure Ruby manifests with node
> 'mynode'")?
>
Actually this was a case of the unit test testing that the node name was
being set correctly in a roundabout way (the name "mynode" is incidental to
how the test is structured). I'll modify the test to be more
straightforward.
> > + it "should allow creation of built-in types" do
> > + write_file('foo.rb', "hostclass 'foo' do file 'test_file', :owner =>
> 'root', :mode => '644' end")
> > + catalog = compile("import 'foo'\ninclude foo")
> > + file = catalog.resource("File[test_file]")
> > + file.should be_a Puppet::Resource
> > + file.type.should == 'File'
> > + file.title.should == 'test_file'
> > + file.exported.should_not be
> > + file.virtual.should_not be
> > + file[:owner].should == 'root'
> > + file[:mode].should == '644'
> > + file[:stage].should be_nil # TODO: is this correct behavior?
> > + end
>
> From the description it sounded like this test was making sure that you
> could create new types, not use existing ones. Yay, English.
>
Yeah, fair point.
>
> > +
> > + it "should allow calling user-defined functions" do
> > + write_file('foo.rb', "hostclass 'foo' do user_func :arg => 'xyz'
> end")
> > + catalog = compile(['define user_func($arg) { notify {"n_$arg": } }',
> > + 'user_func { "one": arg => "two" }'].join("\n"))
> > + catalog.resource("Notify[n_two]").should_not be_nil
> > + catalog.resource("User_func[one]").should_not be_nil
> > + end
>
> Removing the 'write_file' allows this test to still pass. Is it
> really an un-needed vestige, or is something else going on?
>
Whoops, the test as written is completely bogus. This probably was a case
of me being in the middle of doing an experiment, getting distracted, and
never getting back and fixing the test. I'll fix it to reflect my intent.
>
> > +
> > + it "should be properly cached for multiple compiles" do
> > + # Note: we can't test this by calling compile() twice, because
> > + # that sets Puppet[:code], which clears out all cached
> > + # environments.
> > + Puppet[:filetimeout] = 1000
> > + write_file('foo.rb', "hostclass 'foo' do notify('success') end")
> > + Puppet[:code] = "import 'foo'\ninclude foo"
>
> This idiom appeared earlier of having the hostclass named the same as
> the file. While it makes sense, it's also a little confusing in the
> tests. Perhaps they should not have the same name to emphasize exactly
> what is going on?
>
Agreed.
>
> > +
> > + # Compile the catalog and check it
> > + catalog = Dir.chdir(@test_dir) do
> > + Puppet::Parser::Compiler.compile(Puppet::Node.new("mynode"))
> > + end
> > + catalog.resource("Notify[success]").should_not be_nil
> > +
> > + # Secretly change the file to make it invalid. This change
> > + # shouldn't be noticed because the we've set a high
> > + # Puppet[:filetimeout].
> > + write_file('foo.rb', "raise 'should not be executed'")
> > +
> > + # Compile the catalog a second time and make sure it's still ok.
> > + catalog = Dir.chdir(@test_dir) do
> > + Puppet::Parser::Compiler.compile(Puppet::Node.new("mynode"))
> > + end
> > + catalog.resource("Notify[success]").should_not be_nil
>
> Perhaps check that the new file wasn't somehow added to the existing
> one? IOW: "hostclass 'foo' do notify('this should not be here') end"
> catalog.resource("Notify[this should not be here]").should be_nil
>
The intent of the test is to make sure that the new file doesn't get read at
all. I think having it raise an exception is the most effective way to do
that. Your suggestion would weaken the test slightly by allowing for the
possibility that the file gets read and parsed but its contents get ignored.
> > - it "should set any provided block as the type's ruby code"
> > + it "should set any provided block as the type's ruby code" do
> > + Puppet::Resource::Type.any_instance.expects(:ruby_code=)
> > + test_api_call { send(method, "myname") { 'method_result' } }
> > + end
>
> This really only tests that the method was called, not that it actually
> "set[s] any provided block as the type's ruby code", maybe add
> ".with(...)" to the .expects to ensure it gets the correct value? Kinda
> like how you did the test below.
>
Yes, good point. It took some experimenting to figure out the "right" way
to do this :)
Here's the inter-diff. I've squashed it into
http://github.com/stereotype441/puppet/tree/ticket/2.6.x/4716.
diff --git a/lib/puppet/dsl/resource_type_api.rb
b/lib/puppet/dsl/resource_type_api.rb
index 90dd5b2..ecb9141 100644
--- a/lib/puppet/dsl/resource_type_api.rb
+++ b/lib/puppet/dsl/resource_type_api.rb
@@ -17,6 +17,12 @@ class Puppet::DSL::ResourceTypeAPI
nil
end
+ # Note: we don't want the user to call the following methods
+ # directly. However, we can't stop them by making the methods
+ # private because the user's .rb code gets instance_eval'ed on an
+ # instance of this class. So instead we name the methods using
+ # double underscores to discourage customers from calling them.
+
def __mk_resource_type__(type, name, options, code)
klass = Puppet::Resource::Type.new(type, name, options)
diff --git a/spec/integration/parser/ruby_manifest_spec.rb
b/spec/integration/parser/ruby_manifest_spec.rb
index 9ff7805..af110d3 100644
--- a/spec/integration/parser/ruby_manifest_spec.rb
+++ b/spec/integration/parser/ruby_manifest_spec.rb
@@ -42,32 +42,32 @@ describe "Pure ruby manifests" do
end
it "should allow defines" do
- write_file('foo.rb', 'define "foo", :arg do
notify("foo...@name}_#{@arg}") end')
- catalog = compile("import 'foo'\nfoo { instance: arg => 'xyz' }")
- catalog.resource("Notify[foo_instance_xyz]").should_not be_nil
- catalog.resource("Foo[instance]").should_not be_nil
+ write_file('foo.rb', 'define "bar", :arg do
notify("bar...@name}_#{@arg}") end')
+ catalog = compile("import 'foo'\nbar { instance: arg => 'xyz' }")
+ catalog.resource("Notify[bar_instance_xyz]").should_not be_nil
+ catalog.resource("Bar[instance]").should_not be_nil
end
it "should allow node declarations" do
- write_file('foo.rb', ["node 'mynode' do notify('mynode') end",
- "node 'theirnode' do notify('theirnode')
end"].join("\n"))
+ write_file('foo.rb', "node 'mynode' do notify('mynode') end")
catalog = compile("import 'foo'")
- catalog.resource("Notify[mynode]").should_not be_nil
- catalog.resource("Notify[theirnode]").should be_nil
+ node_declaration = catalog.resource("Notify[mynode]")
+ node_declaration.should_not be_nil
+ node_declaration.title.should == 'mynode'
end
it "should allow access to the environment" do
- write_file('foo.rb', ["hostclass 'foo' do",
+ write_file('foo.rb', ["hostclass 'bar' do",
" if environment.is_a?
Puppet::Node::Environment",
" notify('success')",
" end",
"end"].join("\n"))
- compile("import 'foo'\ninclude
foo").resource("Notify[success]").should_not be_nil
+ compile("import 'foo'\ninclude
bar").resource("Notify[success]").should_not be_nil
end
- it "should allow creation of built-in types" do
- write_file('foo.rb', "hostclass 'foo' do file 'test_file', :owner =>
'root', :mode => '644' end")
- catalog = compile("import 'foo'\ninclude foo")
+ it "should allow creation of resources of built-in types" do
+ write_file('foo.rb', "hostclass 'bar' do file 'test_file', :owner =>
'root', :mode => '644' end")
+ catalog = compile("import 'foo'\ninclude bar")
file = catalog.resource("File[test_file]")
file.should be_a Puppet::Resource
file.type.should == 'File'
@@ -80,11 +80,12 @@ describe "Pure ruby manifests" do
end
it "should allow calling user-defined functions" do
- write_file('foo.rb', "hostclass 'foo' do user_func :arg => 'xyz' end")
+ write_file('foo.rb', "hostclass 'bar' do user_func 'name', :arg =>
'xyz' end")
catalog = compile(['define user_func($arg) { notify {"n_$arg": } }',
- 'user_func { "one": arg => "two" }'].join("\n"))
- catalog.resource("Notify[n_two]").should_not be_nil
- catalog.resource("User_func[one]").should_not be_nil
+ 'import "foo"',
+ 'include bar'].join("\n"))
+ catalog.resource("Notify[n_xyz]").should_not be_nil
+ catalog.resource("User_func[name]").should_not be_nil
end
it "should be properly cached for multiple compiles" do
@@ -92,8 +93,8 @@ describe "Pure ruby manifests" do
# that sets Puppet[:code], which clears out all cached
# environments.
Puppet[:filetimeout] = 1000
- write_file('foo.rb', "hostclass 'foo' do notify('success') end")
- Puppet[:code] = "import 'foo'\ninclude foo"
+ write_file('foo.rb', "hostclass 'bar' do notify('success') end")
+ Puppet[:code] = "import 'foo'\ninclude bar"
# Compile the catalog and check it
catalog = Dir.chdir(@test_dir) do
@@ -115,12 +116,12 @@ describe "Pure ruby manifests" do
it "should be properly reloaded when stale" do
Puppet[:filetimeout] = -1 # force stale check to happen all the time
- write_file('foo.rb', "hostclass 'foo' do notify('version1') end")
- catalog = compile("import 'foo'\ninclude foo")
+ write_file('foo.rb', "hostclass 'bar' do notify('version1') end")
+ catalog = compile("import 'foo'\ninclude bar")
catalog.resource("Notify[version1]").should_not be_nil
sleep 1 # so that timestamp will change forcing file reload
- write_file('foo.rb', "hostclass 'foo' do notify('version2') end")
- catalog = compile("import 'foo'\ninclude foo")
+ write_file('foo.rb', "hostclass 'bar' do notify('version2') end")
+ catalog = compile("import 'foo'\ninclude bar")
catalog.resource("Notify[version1]").should be_nil
catalog.resource("Notify[version2]").should_not be_nil
end
diff --git a/spec/unit/dsl/resource_type_api_spec.rb
b/spec/unit/dsl/resource_type_api_spec.rb
index ba396ac..4f4eb7e 100755
--- a/spec/unit/dsl/resource_type_api_spec.rb
+++ b/spec/unit/dsl/resource_type_api_spec.rb
@@ -37,8 +37,8 @@ describe Puppet::DSL::ResourceTypeAPI do
end
it "should set any provided block as the type's ruby code" do
- Puppet::Resource::Type.any_instance.expects(:ruby_code=)
- test_api_call { send(method, "myname") { 'method_result' } }
+ Puppet::Resource::Type.any_instance.expects(:ruby_code=).with { |blk|
blk.call == 'foo' }
+ test_api_call { send(method, "myname") { 'foo' } }
end
it "should add the type to the current environment's known resource
types" do
--
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.
