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