New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add skip_if_offline() #685

Closed
jennybc opened this Issue Nov 15, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@jennybc
Member

jennybc commented Nov 15, 2017

Would a PR to add something like this be accepted? I suppose the ping-ee should be settable, possibly defaulting to google.

skip_if_offline <- (function() {
  offline <- NA
  function() {
    if (is.na(offline)) {
      offline <<- tryCatch(
        is.na(pingr::ping_port("google.com", count = 1, timeout = 1)),
        error = function(e) TRUE
      )
    }
    if (offline) testthat::skip("Offline")
  }
})()

I'm using it in googledrive and adapted it from something @gaborcsardi did in gh.

It means Importing or Suggesting pingr, which doesn't have any dependencies but does require compilation.

@nealrichardson

This comment has been minimized.

Contributor

nealrichardson commented Nov 27, 2017

httptest provides a similar skip_if_disconnected function: https://github.com/nealrichardson/httptest/blob/master/R/offline.R#L16-L23

It uses httr, which testthat does implicitly Suggest via devtools, though if one were dependency averse, something along these lines could surely be accomplished with base R functions.

One nice feature of skip_if_disconnected is that it also wraps skip_on_cran so that network flakiness can't cause a spurious failure on CRAN.

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Nov 27, 2017

@nealrichardson Unfortunately the httr / curl defaults are not appropriate for a check like this.

In particular, if the hostname is in the DNS cache, but it is unreachable, skip_if_disconnected waits until the default curl timeout is over, which is 5 minutes AFAIR. (Turn off the wifi, and then run httptest:::currently_offline() immediately to reproduce.)

Also, AFAIR the curl DNS resolution is not interruptible, so if you have a very slow network or DNS, it will also take a very long time to timeout. (I haven't tried this now, so not sure if it still holds.)

Btw. curl also has has_internet() which performs a forced name service lookup AFAICT, and it does not suffer from the first problem above. It does not check if a particular site is up and serving HTTP, though.

@nealrichardson

This comment has been minimized.

Contributor

nealrichardson commented Nov 27, 2017

@gaborcsardi Thanks, that's good to know about the curl DNS behavior, I hadn't come across that before. Interestingly, I turned off my wifi and ran the full httptest test suite, and it completed in under 2 seconds, hitting 5 skip_if_disconnecteds, so I'm not experiencing a 5 minute curl timeout like you describe. I can't recall ever experiencing that, in fact, and I've done quite a bit of offline test running, but I'll keep it in mind in the event that I do see it.

@jennybc

This comment has been minimized.

Member

jennybc commented Nov 27, 2017

Feels like there are 3 components:

  • How to determine if offline (pingr vs ...)?
  • Using a closure so that offline status is determined once per run vs once per test. Once this lives in a package vs helper.R, seems like the above approach needs adjustment.
  • Whether to bundle up with other skippers, such as skip_on_cran(). That's nice for CRAN itself, but it might have implications re: being alerted to problems when dependencies do revdep checks.
@richfitz

This comment has been minimized.

richfitz commented Dec 19, 2017

I have used variants of this without problems for years:

has_internet <- function() {
  !is.null(suppressWarnings(utils::nsl("www.google.com")))
}

skip_if_no_internet <- function() {
  if (has_internet()) {
    return()
  }
  testthat::skip("no internet")
}

I'm sure there are cases where it can hang badly on poor connections or give false positive/negatives but I work a lot from trains and it seems to work

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Dec 19, 2017

@richfitz yeah, that's better than curl::has_internet, because it does not need an extra package.

@jennybc

This comment has been minimized.

Member

jennybc commented Dec 19, 2017

Do others agree it's a good idea to determine offline status for a test run vs individual tests? I can make a case for that, if necessary.

But under that assumption, if skip_if_offline() is provided by testthat instead of being defined in helper.R, how do you actually accomplish this? How do you remember offline state during a test run, but reset it at the start of a new run?

@rmflight

This comment has been minimized.

rmflight commented Dec 19, 2017

Just in case this goes through, I looked up utils::nsl in the help, can anyone confirm if that function is still not available on Windows? Ideally whatever function is used should be cross-platform.

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Dec 19, 2017

curl has:

The ‘nslookup’ function is similar to ‘nsl’ but works on all
platforms and can resolve ipv6 addresses if supported by the OS.

So yeah, it is not a good option, I am afraid. Thanks for reminding!

@nealrichardson

This comment has been minimized.

Contributor

nealrichardson commented Dec 20, 2017

Re: determining offline status once per run vs. once per test, seems not too difficult to memoize the result one way or another (memoise package or roll-your-own) if you wanted it to only check once per run. But I wonder whether it would be worth the additional complexity. What's your argument for it, @jennybc ?

@jennybc

This comment has been minimized.

Member

jennybc commented Dec 20, 2017

If I'm running all the tests on an API wrapping package, I want a uniform situation wrt to skipping tests due to being offline, because it makes the overall result easier to interpret. I have found that very sporadic internet problems can trigger a skip here and there and then ... I feel like I have to rerun the tests.

But agree it's not clear if the added complexity is worth it.

@nealrichardson

This comment has been minimized.

Contributor

nealrichardson commented Dec 20, 2017

One could make the case that network/API flakiness means you should check connectivity closer to each test to rule out the scenario where the check at the beginning passed but then the connection dropped later in the run. Flaky skips are bad, but flaky failures are worse.

So, what if we could have it both ways? testthat already has skip_if and skip_if_not, so what if the solution for skip_if_offline() were skip_if(is_offline())? If I want to check each time, I can call skip_if(is_offline()) in each test, and if you want to check once at the beginning, you put offline <- is_offline() in your helper.R and call skip_if(offline). No special caching, memoizing, or teardown needed.

Then we've at least reduced the question to how best to determine network status: what goes in is_offline().

jimhester added a commit to jimhester/testthat that referenced this issue Mar 29, 2018

Add skip_if_offline
This is useful for packages which test against online APIs. We use
`curl::nslookup()` rather than `utils::nsl()` because the latter is not
available on Windows.

Fixes r-lib#685

@jimhester jimhester added the wip label Mar 29, 2018

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