You're right, Nigel. I've investigated more deeply and it looks like my test was failing on non-OSX systems because it wasn't stubbing out all the methods it needed to stub out. In particular, it wasn't stubbing out FileTest.file?(Launchd_Overrides). I'll send a follow-up patch to the list as soon as I can.
Markus: it looks like you've already pulled my updated patch into the public 2.6.x branch and closed the ticket. How would you like me to handle the follow-up? My inclination is to reopen the ticket and put the follow-up patch in its own commit under http://github.com/stereotype441/puppet/tree/ticket/2.6.x/4025. On Wed, Sep 29, 2010 at 10:17 AM, Nigel Kersten <[email protected]> wrote: > On Wed, Sep 29, 2010 at 10:11 AM, Paul Berry <[email protected]> wrote: > > Whoops, looks like the spec tests now fail when not running on Mac OSX. > > Since the launchd provider is OSX-specific, I've added a confine to > prevent > > the tests from running on other OSes. > > Is this what we should be doing? > > I was under the impression we were meant to abstract away platform > dependent stuff in our specs? Or is it the case that doing this too > much has actually ended up hiding real issues? > > The specs I've written for OS X would have been much simpler if I knew > they'd only run on OS X :) > > > Squashed this diff into the commit > > (http://github.com/stereotype441/puppet/tree/ticket/2.6.x/4025): > > > > diff --git a/spec/unit/provider/service/launchd_spec.rb > > b/spec/unit/provider/service/launchd_spec.rb > > index 116ba21..05d7697 100755 > > --- a/spec/unit/provider/service/launchd_spec.rb > > +++ b/spec/unit/provider/service/launchd_spec.rb > > @@ -95,6 +95,7 @@ describe provider_class do > > end > > > > describe "when checking whether the service is enabled on OS X 10.6" > do > > + confine "Not running on OSX" => (Facter.value(:operatingsystem) == > > "Darwin") > > it "should return true if the job plist says disabled is true and > the > > global overrides says disabled is false" do > > provider_class.stubs(:get_macosx_version_major).returns("10.6") > > @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" => > > true}]) > > > > On Fri, Sep 24, 2010 at 2:52 PM, Paul Berry <[email protected]> wrote: > >> > >> Modified the launchd provider to use OSX's "plutil" command to read > >> plists. This allows it to handle properly lists in both XML format > >> (which was the default before OSX 10.4) and binary format (which is > >> the default starting with OSX 10.4). > >> > >> Launchd continues to write out propertly lists in XML format. This is > >> not a problem because the operating system is able to understand both > >> formats. > >> > >> Signed-off-by: Paul Berry <[email protected]> > >> --- > >> lib/puppet/provider/service/launchd.rb | 25 > >> ++++++++++++++++--------- > >> spec/unit/provider/service/launchd_spec.rb | 10 +++++----- > >> 2 files changed, 21 insertions(+), 14 deletions(-) > >> > >> diff --git a/lib/puppet/provider/service/launchd.rb > >> b/lib/puppet/provider/service/launchd.rb > >> index 9703595..b296e0a 100644 > >> --- a/lib/puppet/provider/service/launchd.rb > >> +++ b/lib/puppet/provider/service/launchd.rb > >> @@ -38,6 +38,7 @@ Puppet::Type.type(:service).provide :launchd, :parent > => > >> :base do > >> > >> commands :launchctl => "/bin/launchctl" > >> commands :sw_vers => "/usr/bin/sw_vers" > >> + commands :plutil => "/usr/bin/plutil" > >> > >> defaultfor :operatingsystem => :darwin > >> confine :operatingsystem => :darwin > >> @@ -52,6 +53,12 @@ Puppet::Type.type(:service).provide :launchd, :parent > >> => :base do > >> Launchd_Overrides = > >> "/var/db/launchd.db/com.apple.launchd/overrides.plist" > >> > >> > >> + # Read a plist, whether its format is XML or in Apple's "binary1" > >> + # format. > >> + def self.read_plist(path) > >> + Plist::parse_xml(plutil('-convert', 'xml1', '-o', '-', path)) > >> + end > >> + > >> # returns a label => path map for either all jobs, or just a single > >> # job if the label is specified > >> def self.jobsearch(label=nil) > >> @@ -62,8 +69,7 @@ Puppet::Type.type(:service).provide :launchd, :parent > => > >> :base do > >> next if f =~ /^\..*$/ > >> next if FileTest.directory?(f) > >> fullpath = File.join(path, f) > >> - job = Plist::parse_xml(fullpath) > >> - if job and job.has_key?("Label") > >> + if FileTest.file?(fullpath) and job = read_plist(fullpath) > and > >> job.has_key?("Label") > >> if job["Label"] == label > >> return { label => fullpath } > >> else > >> @@ -118,8 +124,11 @@ Puppet::Type.type(:service).provide :launchd, > :parent > >> => :base do > >> def plist_from_label(label) > >> job = self.class.jobsearch(label) > >> job_path = job[label] > >> - job_plist = Plist::parse_xml(job_path) > >> - raise Puppet::Error.new("Unable to parse launchd plist at path: > >> #{job_path}") if not job_plist > >> + if FileTest.file?(job_path) > >> + job_plist = self.class.read_plist(job_path) > >> + else > >> + raise Puppet::Error.new("Unable to parse launchd plist at path: > >> #{job_path}") > >> + end > >> [job_path, job_plist] > >> end > >> > >> @@ -200,9 +209,7 @@ Puppet::Type.type(:service).provide :launchd, > :parent > >> => :base do > >> job_plist_disabled = job_plist["Disabled"] if > >> job_plist.has_key?("Disabled") > >> > >> if self.class.get_macosx_version_major == "10.6": > >> - overrides = Plist::parse_xml(Launchd_Overrides) > >> - > >> - unless overrides.nil? > >> + if FileTest.file?(Launchd_Overrides) and overrides = > >> self.class.read_plist(Launchd_Overrides) > >> if overrides.has_key?(resource[:name]) > >> overrides_disabled = overrides[resource[:name]]["Disabled"] if > >> overrides[resource[:name]].has_key?("Disabled") > >> end > >> @@ -227,7 +234,7 @@ Puppet::Type.type(:service).provide :launchd, > :parent > >> => :base do > >> # versions this is stored in the job plist itself. > >> def enable > >> if self.class.get_macosx_version_major == "10.6" > >> - overrides = Plist::parse_xml(Launchd_Overrides) > >> + overrides = self.class.read_plist(Launchd_Overrides) > >> overrides[resource[:name]] = { "Disabled" => false } > >> Plist::Emit.save_plist(overrides, Launchd_Overrides) > >> else > >> @@ -242,7 +249,7 @@ Puppet::Type.type(:service).provide :launchd, > :parent > >> => :base do > >> > >> def disable > >> if self.class.get_macosx_version_major == "10.6" > >> - overrides = Plist::parse_xml(Launchd_Overrides) > >> + overrides = self.class.read_plist(Launchd_Overrides) > >> overrides[resource[:name]] = { "Disabled" => true } > >> Plist::Emit.save_plist(overrides, Launchd_Overrides) > >> else > >> diff --git a/spec/unit/provider/service/launchd_spec.rb > >> b/spec/unit/provider/service/launchd_spec.rb > >> index 320ee3a..116ba21 100755 > >> --- a/spec/unit/provider/service/launchd_spec.rb > >> +++ b/spec/unit/provider/service/launchd_spec.rb > >> @@ -98,19 +98,19 @@ describe provider_class do > >> it "should return true if the job plist says disabled is true and > the > >> global overrides says disabled is false" do > >> provider_class.stubs(:get_macosx_version_major).returns("10.6") > >> @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" => > >> true}]) > >> - Plist.stubs(:parse_xml).returns({...@resource[:name] => {"Disabled" > => > >> false}}) > >> + @provider.class.stubs(:read_plist).returns({...@resource[:name] => > >> {"Disabled" => false}}) > >> @provider.enabled?.should == :true > >> end > >> it "should return false if the job plist says disabled is false and > >> the global overrides says disabled is true" do > >> provider_class.stubs(:get_macosx_version_major).returns("10.6") > >> @provider.stubs(:plist_from_label).returns(["foo", {"Disabled" => > >> false}]) > >> - Plist.stubs(:parse_xml).returns({...@resource[:name] => {"Disabled" > => > >> true}}) > >> + @provider.class.stubs(:read_plist).returns({...@resource[:name] => > >> {"Disabled" => true}}) > >> @provider.enabled?.should == :false > >> end > >> it "should return true if the job plist and the global overrides > have > >> no disabled keys" do > >> provider_class.stubs(:get_macosx_version_major).returns("10.6") > >> @provider.stubs(:plist_from_label).returns(["foo", {}]) > >> - Plist.stubs(:parse_xml).returns({}) > >> + @provider.class.stubs(:read_plist).returns({}) > >> @provider.enabled?.should == :true > >> end > >> end > >> @@ -182,7 +182,7 @@ describe provider_class do > >> describe "when enabling the service on OS X 10.6" do > >> it "should write to the global launchd overrides file once" do > >> provider_class.stubs(:get_macosx_version_major).returns("10.6") > >> - Plist.stubs(:parse_xml).returns({}) > >> + @provider.class.stubs(:read_plist).returns({}) > >> Plist::Emit.expects(:save_plist).once > >> @provider.enable > >> end > >> @@ -191,7 +191,7 @@ describe provider_class do > >> describe "when disabling the service on OS X 10.6" do > >> it "should write to the global launchd overrides file once" do > >> provider_class.stubs(:get_macosx_version_major).returns("10.6") > >> - Plist.stubs(:parse_xml).returns({}) > >> + @provider.class.stubs(:read_plist).returns({}) > >> Plist::Emit.expects(:save_plist).once > >> @provider.enable > >> end > >> -- > >> 1.7.2 > >> > >> -- > >> 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]<puppet-dev%[email protected]> > . > >> For more options, visit this group at > >> http://groups.google.com/group/puppet-dev?hl=en. > >> > > > > -- > > 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]<puppet-dev%[email protected]> > . > > For more options, visit this group at > > http://groups.google.com/group/puppet-dev?hl=en. > > > > > > -- > nigel > > -- > 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]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- 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.
