On Sat, Jan 16, 2010 at 7:12 PM, Nigel Kersten <[email protected]> wrote:
> > > On Sat, Jan 16, 2010 at 5:47 PM, Bruce Williams > <[email protected]>wrote: > >> >> >> On Sat, Jan 16, 2010 at 4:26 PM, Bruce Williams >> <[email protected]>wrote: >> >>> >>> On Jan 16, 2010, at 12:10 PM, Nigel Kersten <[email protected]> wrote: >>> >>> >>> >>> On Sat, Jan 16, 2010 at 11:47 AM, < <[email protected]> >>> [email protected]> wrote: >>> >>>> From: Bruce Williams < <[email protected]>[email protected]> >>>> >>>> The failures were caused by a couple issues that may have been limited >>>> to OSX (possibly only SL). >>>> * The absence of /bin/true caused a couple Resolution.exec tests to >>>> fail. Instead of expecting a binary to exist, the exists? check is now >>>> stubbed. >>>> >>> >>> We can't just use /usr/bin/true instead? >>> >>> >>> Sure we could, but there's no reason for that test to shell out at all in >>> any case (all streams are mocked); it's not the purpose of the test. >>> >>> >>> >>>> * Changes to system_profiler output or the Resolution.exec result type >>>> caused in an exception with the plist parser (which would cause >>>> Factor.list, among other things, to fail); this is now handled >>>> gracefully. >>>> >>> >>> Do we have examples of system_profiler -xml output changing output >>> format? Apple aren't meant to do that, they supposedly guarantee the >>> xml/plist output formats do not change. >>> >>> >>> >>> The exec return value is an array (which causes issues with the plist >>> parser); I agree it's far more likely this case is due to a change in facter >>> vs system_profiler. >>> >>> >>> >>>> >>>> Signed-off-by: Bruce Williams < <[email protected]> >>>> [email protected]> >>>> --- >>>> .gitignore | 1 + >>>> Rakefile | 4 +--- >>>> lib/facter/util/macosx.rb | 17 ++++++++--------- >>>> lib/facter/util/resolution.rb | 14 +++++++++----- >>>> spec/spec.opts | 1 + >>>> spec/unit/interfaces.rb | 12 ++---------- >>>> spec/unit/operatingsystem.rb | 4 ---- >>>> spec/unit/selinux.rb | 4 ---- >>>> spec/unit/util/resolution.rb | 38 >>>> +++++++++++++++++++++++--------------- >>>> spec/unit/virtual.rb | 3 --- >>>> 10 files changed, 45 insertions(+), 53 deletions(-) >>>> create mode 100644 .gitignore >>>> >>>> diff --git a/.gitignore b/.gitignore >>>> new file mode 100644 >>>> index 0000000..4ebc8ae >>>> --- /dev/null >>>> +++ b/.gitignore >>>> @@ -0,0 +1 @@ >>>> +coverage >>>> diff --git a/Rakefile b/Rakefile >>>> index e3549ca..d1b7e56 100644 >>>> --- a/Rakefile >>>> +++ b/Rakefile >>>> @@ -47,9 +47,7 @@ end >>>> Rake::GemPackageTask.new(spec) do |pkg| >>>> end >>>> >>>> -task :default do >>>> - sh %{rake -T} >>>> -end >>>> +task :default => :spec >>>> >>>> desc "Run the specs under spec/" >>>> task :spec do >>>> diff --git a/lib/facter/util/macosx.rb b/lib/facter/util/macosx.rb >>>> index 6754f18..7300afe 100644 >>>> --- a/lib/facter/util/macosx.rb >>>> +++ b/lib/facter/util/macosx.rb >>>> @@ -25,7 +25,7 @@ module Facter::Util::Macosx >>>> # by looking at the _name key of the _items dict for each _dataType >>>> >>>> def self.profiler_xml(data_field) >>>> - Facter::Util::Resolution.exec("/usr/sbin/system_profiler -xml >>>> #{data_field}") >>>> + Array(Facter::Util::Resolution.exec("/usr/sbin/system_profiler >>>> -xml #{data_field}")).join("\n") >>>> >>> >>> We really shouldn't have to be doing this sort of join with xml output. >>> Is there an example of where it was blowing up? >>> >>> Oops, I missed this question in my last response. >> >> Here's the blowup (easily reproduced with Facter.list on my machine at >> master HEAD): >> >> ./lib/facter/macosx.rb:28: undefined method `each' for nil:NilClass >> (NoMethodError) >> from ./lib/facter/util/loader.rb:73:in `load' >> from ./lib/facter/util/loader.rb:73:in `load_file' >> from ./lib/facter/util/loader.rb:38:in `load_all' >> >> This is caused by the following at lib/facter/macosx.rb:28 returning `nil` >> >> Facter::Util::Macosx.hardware_overview >> >> Which is occurring because the method is rescuing the following exception: >> >> ./lib/facter/util/plist/parser.rb:81:in `exists?': can't convert Array >> into String (TypeError) >> from ./lib/facter/util/plist/parser.rb:81:in `parse' >> from ./lib/facter/util/plist/parser.rb:28:in `parse_xml' >> from ./lib/facter/util/macosx.rb:33:in `intern_xml' >> from ./lib/facter/util/macosx.rb:39:in `profiler_data' >> >> This is occurring because Facter::Util::Resolution.exec returns an array >> of lines, which Plist.parse_xml doesn't seem to appreciate very much at all. >> > > So this is normally fine. I do lots of work with plistlib and arrays. Are > you sure that whatever is being queried shouldn't in fact *be* multi-valued > and facter is mistaken in assuming it should be a single value? > All I know is the exec is returning the XML document from system_profiler split up by lines (eg, the declaration as the first), and the Plist parser is expecting the entire document. Sure, hardware_overview can (should) be multiple values, but that's the result of the parse. > > >> >> While I agree with the "we shouldn't have to be doing this sort of join" >> sentiment in general, normalizing in the utility seemed like the most >> straightforward, unobtrusive, backwards-compatible way to provide the Plist >> parser with a string it can handle (which should be equivalent to the >> original output, pre-split). I am very, very happy to entertain any >> alternative -- my goal here being to get the tests passing so I tackle more >> interesting problems on steady ground. ;-) >> > > understood. :) I've been pretty much absent from anything other than work > for the last few weeks... but should be starting to return to sanity in the > near future and I'll have a look at this. > > >> >> Cheers, >> Bruce >> -- >> Bruce Williams >> http://reductivelabs.com >> >>> >>> >>> >>> >>>> end >>>> >>>> def self.intern_xml(xml) >>>> @@ -35,14 +35,13 @@ module Facter::Util::Macosx >>>> >>>> # Return an xml result, modified as we need it. >>>> def self.profiler_data(data_field) >>>> - begin >>>> - return nil unless parsed_xml = >>>> intern_xml(profiler_xml(data_field)) >>>> - return nil unless data = parsed_xml[0]['_items'][0] >>>> - data.delete '_name' >>>> - data >>>> - rescue >>>> - return nil >>>> - end >>>> + return nil unless parsed_xml = >>>> intern_xml(profiler_xml(data_field)) >>>> + return nil unless data = parsed_xml[0]['_items'][0] >>>> + data.delete '_name' >>>> + data >>>> + rescue >>>> + # If there's an exception, this field isn't available or is >>>> + # incorrectly formatted. >>>> end >>>> >>>> def self.hardware_overview >>>> diff --git a/lib/facter/util/resolution.rb >>>> b/lib/facter/util/resolution.rb >>>> index f6afce6..862ea37 100644 >>>> --- a/lib/facter/util/resolution.rb >>>> +++ b/lib/facter/util/resolution.rb >>>> @@ -39,26 +39,30 @@ class Facter::Util::Resolution >>>> path = binary >>>> end >>>> >>>> - return nil unless FileTest.exists?(path) >>>> + return nil unless binary_exists?(path) >>>> end >>>> >>>> out = nil >>>> err = nil >>>> begin >>>> - Open3.popen3(code) do |stdin,stdout,stderr| >>>> - out = self.parse_output(stdout.readlines) >>>> - err = self.parse_output(stderr.readlines) >>>> + Open3.popen3(code) do |stdin, stdout, stderr| >>>> + out = self.parse_output(stdout.readlines) >>>> + err = self.parse_output(stderr.readlines) >>>> end >>>> rescue => detail >>>> $stderr.puts detail >>>> return nil >>>> end >>>> Facter.warn(err) >>>> - >>>> + >>>> return nil if out == "" >>>> out >>>> end >>>> >>>> + def self.binary_exists?(path) >>>> + File.exists?(path) >>>> + end >>>> + >>>> def self.parse_output(output) >>>> return nil unless output and output.size > 0 >>>> result = output.collect{|line| line.chomp } >>>> diff --git a/spec/spec.opts b/spec/spec.opts >>>> index 695852c..91cd642 100644 >>>> --- a/spec/spec.opts >>>> +++ b/spec/spec.opts >>>> @@ -3,3 +3,4 @@ s >>>> --colour >>>> --loadby >>>> mtime >>>> +--backtrace >>>> diff --git a/spec/unit/interfaces.rb b/spec/unit/interfaces.rb >>>> index 49d5d1f..bf8d39a 100644 >>>> --- a/spec/unit/interfaces.rb >>>> +++ b/spec/unit/interfaces.rb >>>> @@ -2,18 +2,10 @@ >>>> >>>> require File.dirname(__FILE__) + '/../spec_helper' >>>> >>>> -require 'facter' >>>> - >>>> describe "Per Interface IP facts" do >>>> - before do >>>> - Facter.loadfacts >>>> - end >>>> - >>>> it "should replace the ':' in an interface list with '_'" do >>>> - # So we look supported >>>> - Facter.fact(:kernel).stubs(:value).returns("SunOS") >>>> - >>>> - Facter::Util::IP.expects(:get_interfaces).returns %w{eth0:1 >>>> eth1:2} >>>> + Facter.fact(:kernel).stubs(:value => "SunOS") >>>> + Facter::Util::IP.stubs(:get_interfaces => %w{eth0:1 eth1:2}) >>>> Facter.fact(:interfaces).value.should == %{eth0_1,eth1_2} >>>> end >>>> end >>>> diff --git a/spec/unit/operatingsystem.rb b/spec/unit/operatingsystem.rb >>>> index de86230..ab21682 100644 >>>> --- a/spec/unit/operatingsystem.rb >>>> +++ b/spec/unit/operatingsystem.rb >>>> @@ -1,9 +1,5 @@ >>>> -#!/usr/bin/env ruby >>>> - >>>> require File.dirname(__FILE__) + '/../spec_helper' >>>> >>>> -require 'facter' >>>> - >>>> describe "Operating System fact" do >>>> >>>> before do >>>> diff --git a/spec/unit/selinux.rb b/spec/unit/selinux.rb >>>> index 8afa463..5e54c29 100644 >>>> --- a/spec/unit/selinux.rb >>>> +++ b/spec/unit/selinux.rb >>>> @@ -1,9 +1,5 @@ >>>> -#!/usr/bin/env ruby >>>> - >>>> require File.dirname(__FILE__) + '/../spec_helper' >>>> >>>> -require 'facter' >>>> - >>>> describe "SELinux facts" do >>>> >>>> >>>> diff --git a/spec/unit/util/resolution.rb b/spec/unit/util/resolution.rb >>>> index 27cb150..ee835e3 100755 >>>> --- a/spec/unit/util/resolution.rb >>>> +++ b/spec/unit/util/resolution.rb >>>> @@ -233,27 +233,35 @@ describe Facter::Util::Resolution do >>>> >>>> # It's not possible, AFAICT, to mock %x{}, so I can't really test >>>> this bit. >>>> describe "when executing code" do >>>> - it "should fail if any interpreter other than /bin/sh is >>>> requested" do >>>> - lambda { Facter::Util::Resolution.exec("/something", >>>> "/bin/perl") }.should raise_error(ArgumentError) >>>> + >>>> + describe "using an interpreter other than /bin/sh" do >>>> + it "should fail" do >>>> + lambda { Facter::Util::Resolution.exec("/something", >>>> "/bin/perl") }.should raise_error(ArgumentError) >>>> + end >>>> end >>>> >>>> - it "should produce stderr content as a warning" do >>>> - stdout = stdin = stub('fh', :readlines => ["aaa\n"]) >>>> - stderr = stub('stderr', :readlines => %w{my content}) >>>> - Open3.expects(:popen3).with("/bin/true").yields(stdin, >>>> stdout, stderr) >>>> + describe "using /bin/sh as the interpreter" do >>>> >>>> - Facter.expects(:warn).with(['my','content']) >>>> - Facter::Util::Resolution.exec("/bin/true").should == 'aaa' >>>> - end >>>> + it "should produce stderr content as a warning" do >>>> + stdout = stdin = stub('fh', :readlines => ["aaa\n"]) >>>> + stderr = stub('stderr', :readlines => %w{my content}) >>>> + Open3.expects(:popen3).with("/bin/true").yields(stdin, >>>> stdout, stderr) >>>> + Facter.expects(:warn).with(['my','content']) >>>> + >>>> >>>> Facter::Util::Resolution.expects(:binary_exists?).with('/bin/true').returns(true) >>>> + Facter::Util::Resolution.exec("/bin/true").should == >>>> 'aaa' >>>> + end >>>> >>>> - it "should produce nil as a warning if nothing is printed to >>>> stderr" do >>>> - stdout = stdin = stub('fh', :readlines => ["aaa\n"]) >>>> - stderr = stub('stderr', :readlines => []) >>>> - Open3.expects(:popen3).with("/bin/true").yields(stdin, >>>> stdout, stderr) >>>> + it "should produce nil as a warning if nothing is printed >>>> to stderr" do >>>> + stdout = stdin = stub('fh', :readlines => ["aaa\n"]) >>>> + stderr = stub('stderr', :readlines => []) >>>> + Open3.expects(:popen3).with("/bin/true").yields(stdin, >>>> stdout, stderr) >>>> + Facter.expects(:warn).with(nil) >>>> + >>>> >>>> Facter::Util::Resolution.expects(:binary_exists?).with('/bin/true').returns(true) >>>> + Facter::Util::Resolution.exec("/bin/true").should == >>>> 'aaa' >>>> + end >>>> >>>> - Facter.expects(:warn).with(nil) >>>> - Facter::Util::Resolution.exec("/bin/true").should == 'aaa' >>>> end >>>> + >>>> end >>>> >>>> describe "when parsing output" do >>>> diff --git a/spec/unit/virtual.rb b/spec/unit/virtual.rb >>>> index cc72ffc..b11927c 100644 >>>> --- a/spec/unit/virtual.rb >>>> +++ b/spec/unit/virtual.rb >>>> @@ -1,8 +1,5 @@ >>>> require File.dirname(__FILE__) + '/../spec_helper' >>>> >>>> -require 'facter' >>>> -require 'facter/util/virtual' >>>> - >>>> describe "Virtual fact" do >>>> >>>> after do >>>> -- >>>> 1.6.0.4 >>>> >>>> >>>> -- >>>> 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]> >>>> [email protected]. >>>> To unsubscribe from this group, send email to >>>> <puppet-dev%[email protected]> >>>> [email protected]. >>>> For more options, visit this group at >>>> <http://groups.google.com/group/puppet-dev?hl=en> >>>> 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]. >>> 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. > > -- Bruce Williams http://reductivelabs.com--
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.
