Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix a typo in the unit test for Net::Ping::External and change the implementation for Windows #33

Merged
merged 4 commits into from Mar 28, 2014

Conversation

Projects
None yet
2 participants
Contributor

bcandrea commented Mar 27, 2014

I had a typo in the test code for timeouts in Net::Ping::External; that was making the test pass even if the timeout wasn't enforced. I reimplemented it using the '-w' option of the Windows version of ping (hoping to avoid thread/process management entirely), and it seems to work now.

bcandrea added some commits Mar 27, 2014

@bcandrea bcandrea Merge pull request #1 from djberg96/master
Merge back changes from djberg96
c034875
@bcandrea bcandrea Fix a typo in the Net::Ping::External test
This typo had the effect of setting the expected
ping duration to 5.5s instead of 1.5s. Since in Windows
the defaul ping timeout is 4s, the test always passed.
ef80ced
@bcandrea bcandrea Avoid implementing timeout on Windows
The -w option of ping on Windows has the same effect
of an external timeout.
9fe37ec
@bcandrea bcandrea Merge branch 'feature/fix-win32-external-ping' into develop c805658

Come to think of it, is there any reason not to just use -W on *nix platforms?

Contributor

bcandrea commented Mar 28, 2014

Well, just a few minor annoyances. First, the timeout option might be different in hpux/solaris (I have no clue, to be honest). More importantly, it seems that things like

ping -W 0.3 -c 1 10.0.0.0

work correctly on OSX, but not on Linux (non-integer wait times are simply ignored). It might not be an issue, but in some cases I really wanted to have the option of dropping pings after a very short time - that was actually the reason why I started to test timeouts in the first place :)

Owner

djberg96 commented Mar 28, 2014

Hm, this code wasn't really meant to be much more than a thin wrapper around your system's timeout command, so I worry about overcomplicating the code when we could just use -W. Are timeouts less than 1 second really useful in practice?

I'll double check Solaris and BSD to see if they have an option.

Anyway, in the meantime, I'll merge this and we can adapt as needed.

@djberg96 djberg96 added a commit that referenced this pull request Mar 28, 2014

@djberg96 djberg96 Merge pull request #33 from bcandrea/develop
Fix a typo in the unit test for Net::Ping::External and change the implementation for Windows
a4ae2bd

@djberg96 djberg96 merged commit a4ae2bd into djberg96:master Mar 28, 2014

1 check passed

default The Travis CI build passed
Details
Contributor

bcandrea commented Mar 28, 2014

Actually, the semantics of the options is different even between OSX and linux:

OSX:

-t timeout
             Specify a timeout, in seconds, before ping exits regardless of 
             how many packets have been received.
[...]
-W waittime
             Time in milliseconds to wait for a reply for each packet sent.  
             If a reply arrives later, the packet is not printed as
             replied, but considered as replied when calculating statistics.

Linux:

-t  ttl ping only.  Set the IP Time to Live.
-w  deadline
              Specify a timeout, in seconds, before ping exits regardless of 
              how many packets have been sent or  received.  In  this
              case  ping  does  not  stop after count packet are sent, it waits 
              either for deadline expire or until count probes are
              answered or for some error notification from network.
Contributor

bcandrea commented Mar 28, 2014

In practice 1 sec is OK, it really doesn't matter (I was just a bit worried about being hit by subtle differences between OSes). Thanks!

Owner

djberg96 commented Mar 28, 2014

Ok, maybe it's best to keep your approach in the name of uniformity. That's what the original Timeout block did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment