On Jan 16, 2010, at 12:10 PM, Nigel Kersten <[email protected]> wrote:
On Sat, Jan 16, 2010 at 11:47 AM, <[email protected]> wrote: From: Bruce Williams <[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 causeFactor.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]> --- .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 _dataTypedef 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?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_overviewdiff --git a/lib/facter/util/resolution.rb b/lib/facter/util/ resolution.rbindex 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 enddiff --git a/spec/unit/operatingsystem.rb b/spec/unit/ operatingsystem.rbindex 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" dodiff --git a/spec/unit/util/resolution.rb b/spec/unit/util/ resolution.rbindex 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].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 .-- 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.
