On Thursday, November 8, 2012 8:27:39 AM UTC+11, Jeff McCune wrote:
> Yes, I think we should leave 11612 as it is. Merged pending release is a
> state that is considered "closed," but it's a soft form of being closed.
> The ticket changes to the closed state when the change is actually
> released. Would you mind creating additional tickets for the following
> "still to do" items? One ticket for each actionable issue will help me
> streamline the changes through our system.
>
Will do.
> Still to do, add physicalprocessorcount (this is also a headache on HP-UX)
>> and add Hongbo Hu's bugfix for dealing with HP-UX NICs that have an
>> asterisk after them. This could probably be put in straight away - but the
>> question is are we going to use this opportunity to write proper unit tests
>> for ip.rb? That might take me, personally, too far afield.
>>
>
> I'd be happy to write the unit tests for ip.rb, but I do this sort of work
> when I have the opportunity, which is not all that frequently. If you
> could isolate the potential blocker of unit tests in it's own issue, it'll
> help keep the other issues streamlined.
>
Happy to help out where I can. I'm still new to this so here's the
problem as I see it -
The spec test for the interface list on HP-UX is -
it "should return a list three interfaces on HP-UX with three interfaces
multiply reporting" do
hpux_netstat = my_fixture_read("hpux_netstat_all_interfaces")
Facter::Util::IP.stubs(:get_all_interface_output).returns(hpux_netstat)
Facter::Util::IP.get_interfaces().should == ["lan1", "lan0", "lo0"]
end
The fixture hpux_netstat_all_interfaces is, surprisingly, not the output of
netstat -in on HP-UX, but rather what the author believed the
get_all_interface_output method would return.
Now, here is the get_all_interface_output method -
def self.get_all_interface_output
case Facter.value(:kernel)
when 'Linux', 'OpenBSD', 'NetBSD', 'FreeBSD', 'Darwin', 'GNU/kFreeBSD',
'DragonFly'
output = %x{/sbin/ifconfig -a 2>/dev/null}
when 'SunOS'
output = %x{/usr/sbin/ifconfig -a}
when 'HP-UX'
output = %x{/bin/netstat -in | sed -e 1d}
when 'windows'
output = %x|#{ENV['SYSTEMROOT']}/system32/netsh.exe interface ip show
interface|
output += %x|#{ENV['SYSTEMROOT']}/system32/netsh.exe interface ipv6
show interface|
end
output
end
The supplied fixture for HP-UX is the output of netstat -in |sed -e 1d -
root@myhost:(ip)> cat hpux_netstat_all_interfaces
lan1 1500 192.168.100.0 192.168.100.182 12964 0 900 0 0
lan0 1500 192.168.100.0 192.168.100.181 12964 0 715 0 0
lo0 4136 127.0.0.0 127.0.0.1 98 0 98 0 0
However the author has not considered the case of netstat -in output that
has asterisks after the interface as in -
root@myhost:()> netstat -in |sed -e 's/[0-9]/X/g'
Name Mtu Network Address Ipkts Ierrs Opkts Oerrs
Coll
lanX* XXXX XX.XX.XX.X XX.XX.XX.X XXX X XXX X X
lanX XXXX XXX.XX.X.X XXX.XX.X.X XXXXXXXXXX X XXXXXXXXX
X X
loX XXXX XXX.X.X.X XXX.X.X.X XXXXXXX X XXXXXXX X X
And as Hongbo Hu showed, you can solve the problem easily enough by simply
extending the sed command to delete not only the first line, but also any
asterisks.
>From a unit test perspective, however, the code would need to be changed in
order to reproduce the failing behaviour.
My feeling is we would need to rewrite this code with shim methods for the
commands that are executed externally, collect a good sample of outputs
from the various platforms that the code executes on, and then stub all
these calls to external commands in completely new unit tests.
Next problem is that the code uses a big case statement of the kind Andy
said he doesn't like. So, I guess we would also want to completely rewrite
the method - and that would affect all platforms.
Finally, the whole of ip.rb and ip_spec.rb is written in the same way. So,
my conclusion is - since the code's working it's probably low priority and
when we really have lots of spare time then the whole thing needs to be
rewritten, code, unit tests and all. So it's not obvious to me that it's
worth the effort of changing this method and implementing spec tests if,
really, the whole thing needs to be rewritten.
People with more experience on the project might come to a different
conclusion of course.
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To view this discussion on the web visit
https://groups.google.com/d/msg/puppet-dev/-/d7PAsRVtRxcJ.
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.
