Skip to content

Adds extra tests to check timeouts are enforced (and finds some bugs) #34

Closed
wants to merge 49 commits into from

3 participants

@ianheggie

I noticed some work has been done on timeouts, so I added extra tests to check timeouts are enforced.

This brings to light two problems:
1. http ping works differently between ruby 1.9.2 and ruby 1.9.3 (1.9.2 doesn't wait for delayed response from a http server, 1.9.3 does) - I am assuming a http ping should discover where a load balancer etc accepts a connection but then for whatever reason a response is not sent back in a timely fashion (server farm hung etc)
2. JRuby - http ping waits forever for http server to respond

I could hive the two problems off into "allowed failure" tests, so the tests pass if no one breaks what already works. (And then add issues that point to the failing tests) Do you want me to do that?

Note - to be complete, the test should be expanded to check connection to an IP#/port that has a drop packet firewall rule (which normally hangs tcp connections for a while). This would require adding a firewall rule to the travis box - what do you think?

I think it is doable, but we need something like BLACK_HOLE_HOST_PORT=192.168.0.122:3333 passed in to the tests, so if set then the tests can assume that that host:port combination will silently drop any packets sent to it, thus causing a tcp connection to hang for some period of time.

I am unsure if we can rely on the IP# in the reserved for document range ( http://tools.ietf.org/html/rfc5737 ) to drop packets rather than returning a connection refused icmp packet. If not, then i would need to consider adding a firewall rule to the test host to drop the packets.

p.s. simple test from my location works:

$ time telnet 192.0.2.12
Trying 192.0.2.12...
telnet: Unable to connect to remote host: Connection timed out

real    2m7.251s
user    0m0.006s
sys 0m0.000s
and others added some commits Feb 25, 2014
@djberg96 Added author to README. fecd90d
@djberg96 Commented out a test that usually failed. 3ccaee9
@ianheggie ianheggie Add travis tests for various ruby versions, with 1.8.7 and jruby mark…
…ed as allowed failures
e391c27
@ianheggie ianheggie Commented out jruby 18mode and 19mode till it is fixed so
we don't repeat three copies of it freezing for 10 minutes
1c0a6f7
@djberg96 Merge pull request #29 from ianheggie/add_travis_tests
Add travis tests for various ruby versions, with 1.8.7 and jruby marked ...
55d00af
@ianheggie ianheggie Add ICMP tests to travis-ci (refactored exclusions to work
with rvmsudo);
Drop ruby 1.8.7 test;
Fix ping of 192.168.0.1 to use 127.0.0.1 instead;
Move handling of EXCLUDE_ICMP_TEST to the same place as other excludes;
9a0748f
@ianheggie ianheggie Add to Known Issues in README based on testing results 6c59c70
@djberg96 Merge pull request #31 from ianheggie/add_icmp_tests_to_travis
Add icmp tests to travis, drop ruby 1.8.7, note known issues discovered in testing
c699a8a
@bcandrea bcandrea Fixed timeouts in Net::Ping::External
Using Timeout.timeout with Open3 is not an option with Ruby > 1.9.3
(see https://bugs.ruby-lang.org/issues/5487). Implemented a workaround
based on the code at https://gist.github.com/lpar/1032297
77fd65a
@bcandrea bcandrea Update travis.yml to trigger CI run
This update should trigger a Travis CI build on different Rubies.
6ae1b88
@djberg96 Merge pull request #32 from bcandrea/develop
Fixed timeouts in Net::Ping::External
61cd0a4
@djberg96 Don't use read_nonblock on Windows. bb83bc9
@ianheggie ianheggie Check icmp timeout 03ced34
@ianheggie ianheggie Add timeout tests for all classes d60b806
@ianheggie ianheggie Add echo commands, and timeout test for http 997b2c6
@ianheggie ianheggie Adjust timeout tests d42a080
@ianheggie ianheggie Commnent on delay 0109df7
@ianheggie ianheggie Extra testing for timeout 6aee3c6
@ianheggie ianheggie reduce time to timeout+4 that failing tests have to wait 30874ff
@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
@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 Remove comment about win32-open3, add note about minimum Ruby version. 666474e
@ianheggie ianheggie Merge remote-tracking branch 'upstream/master' into check_timeout
Conflicts:
	lib/net/ping/external.rb
bf69118
@ianheggie ianheggie Add extra rvm versions;
Add IGNORE_HTTP_PING_DOES_NOT_WAITS_FOR_RESPONSE_BUG and
IGNORE_HTTP_PING_DOES_NOT_TIMEOUT_BUG to test those bugs with
failure allowed;
Reorganise .travis.yml to make intent clearer
7988a33
@ianheggie ianheggie Refactor tests to line up with existing example in external 8299df2
@ianheggie ianheggie Simplify and clarify tests for unreachable and slow servers b8e643e
@ianheggie

Just doing a bit of reorganising -please hold off merging - I will add another comment when done.

@djberg96
Owner

I'd rather not alter the API just for tests.

@ianheggie

Understand - I went another way anyway ... i'm experimenting with a few things in the tests as you may have noticed ... I will add a comment here when it is stable ... would you prefer I left it as a mass of commits or squashed it up (when im finished)?

@ianheggie

I have made a squashed commit ( #35 ), which is my preference, but both have identical changes, this one just has all the details of aborted attempts and mistakes I made, which I am happy not to add to your history! grin

As noted on the other pull request, once you choose, then I am happy to help with some of the bugs I found.

@djberg96
Owner

Upon further reflection, I've decided to go back to just using the command line arguments. I think I lost sight of the purpose of the External class, which is to just wrap an external timeout. If folks need more granular timeout control, I think they shouldn't use an external ping to begin with.

I don't want you to think I don't appreciate your contribution - I do! And you will still get credit for spotting the useless Timeout wrapper and providing a solution.

@ianheggie ianheggie closed this Apr 2, 2014
@ianheggie ianheggie deleted the ianheggie:check_timeout branch Apr 2, 2014
@ianheggie

I don't see a conflict between adding tests and narrowing the purpose of the External class - the tests should test an example of the behaviour that you want.

I noted the change for external ping under windows - linux also has an equivalent comand arg for ping - I wasn't sure about all the other variations.

If the example isn't correct then we adjust the tests, if the tests do reflect the behaviour you want, and for some versions of ruby they fail then there is the choice to either fix the code (if possible, eg icmp ping of an unreachable route/host) or mark it unsupported (eg some of the JRuby issues).

Note - I have closed the verbose branch and left the one that squashed the tests together, since I have found the fix for icmp route unreachable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.