On Wed, 22 Sep 2010 16:38:59 -0700, Paul Berry wrote: > Date: Wed, 22 Sep 2010 16:38:59 -0700 > From: Paul Berry <[email protected]> > To: [email protected] > Subject: Re: [Puppet-dev] [PATCH/puppet 1/1] [#4716] ResourceTypeAPI > exposes > implementation details that are likely to change > Message-ID: <[email protected]> > > 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. >
+1 with the above changes. -- Jacob Helwig
signature.asc
Description: Digital signature
