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? > > 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].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
