Skip to content
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

Do not use tests if no internet #379

Closed
PMassicotte opened this issue Feb 20, 2021 · 14 comments
Closed

Do not use tests if no internet #379

PMassicotte opened this issue Feb 20, 2021 · 14 comments

Comments

@PMassicotte
Copy link
Owner

Dear maintainer,
Please see the problems shown on
https://cran.r-project.org/web/checks/check_results_gtrendsR.html.
Please correct before 2021-03-03 to safely retain your package on CRAN.
It seems we need to remind you of the CRAN policy:
'Packages which use Internet resources should fail gracefully with an informative message
if the resource is not available or has changed (and not give a check warning nor error).'
This needs correction whether or not the resource recovers.
The CRAN Team

@PMassicotte
Copy link
Owner Author

@eddelbuettel Hi Dirk. This is the simplest way I found to check if there is access to the internet before performing the test.

@eddelbuettel
Copy link
Collaborator

The predicate

 if (!) {

looks good to me! And we already ensure we always have curl. Now, as a matter of process what I often do is to test at the of each test file a la (mocked up now)

noDNS <-  is.null(curl::nslookup("google.com", error = FALSE))
# other predicates ...

if (noDNS) exit_file("Skipping tests for lack of internet")
# ... remainder

which would allow us to run some tests but not others.

Now, looking at ?nslookup in a session with curl it even shows has_internet():

> has_internet()
[1] TRUE
> 

so the test could be even simpler...

@PMassicotte
Copy link
Owner Author

Sounds good about the has_internet. I have not checked that. Let me change it. I will also check internet before each test.

@eddelbuettel
Copy link
Collaborator

Nicely done!

@PMassicotte
Copy link
Owner Author

Thank you, but apparently still failing. I have forwarded you the information.

@eddelbuettel
Copy link
Collaborator

Hm, per https://win-builder.r-project.org/incoming_pretest/gtrendsR_1.4.8_20210220_200543/Debian/00check.log you still have the wrong tests/tinytest.R with an internet test there. Or am I missing something?

Will try here. Have never ever seen that curl error.

@PMassicotte
Copy link
Owner Author

True, I submitted by error before removing this line (which is now ok). But, this is just checking if internet is available before launching the whole series of tests. Do you think this could be the problem?

@eddelbuettel
Copy link
Collaborator

I am very confused by this. It also passes here. I just sent it to RHub as well (on one of the Debian boxen). Running now:

> check(path="gtrendsR_1.4.8.tar.gz", platform="debian-gcc-release", email="edd@debian.org")
─  Uploading packagePreparing build, see status at
   https://builder.r-hub.io/status/gtrendsR_1.4.8.tar.gz-745229d17e2c48249fe24ed1a1cc47a8Build startedDownloading and unpacking package fileQuerying system requirements
...

On the margin I would recommend skipping tests at CRAN and else where unless an environment variable is set. Say, call it GTRENDS_RUN_ALL_TESTS and then add in failing tinytest files before the failure a simple

if (Sys.getenv("GTRENDS_RUN_ALL_TESTS", "") == "") exit_file("Skipping tests in limited run.")

@PMassicotte
Copy link
Owner Author

Everything is also fine on my side (including RHub). I like your idea. Can you implement it?

@eddelbuettel
Copy link
Collaborator

Sure. Do you have a preferred name for the environment variable? (And for examples you can grep in the tinytest dir for Rcpp where I think I used two for 'more tests' and 'loud tests' that put a lot of stuff on stdout).

@eddelbuettel
Copy link
Collaborator

Yes, same for me. If we cannot reproduce the issue we should skip it. You "just" have to remember to set the var at your end when developing. (And maybe set it at Travis or GitHub.)

BTW I can also send you a simple PR to do what we did before at Travis now at GitHub with Actions. There is always more but I like to keep it simple and quick...

@PMassicotte
Copy link
Owner Author

I see that you used

if (Sys.getenv("RunAllRcppTests") != "yes") exit_file("Set 'RunAllRcppTests' to 'yes' to run.")

I do not have a preferred name, hummm, something as simple as RunAllTests?

Also ok for your PR, it is a clever idea. While there, any benefits/thoughts on eventually switching to Actions?

@eddelbuettel
Copy link
Collaborator

I think I would embed gtrendsR in the name. Otherwise mostly a preference over mixedCase and ALL_UPPER.

As for Actions, turns out I was in fact wrong and I had no choice as I ran out of credits at Travis. I don't mind Actions, I am not as religious about it as some other people. I blogged about my set up -- now basically the same script at Travis, GitHub Actions or Azure Pipelines. So if one is annoying I can pick the other.

I can deal with both over morning coffee tomorrow. Good / soon enough?

@PMassicotte
Copy link
Owner Author

Sounds prefect my friend. As usual, thank you for your help, much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants