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

dontrun or donttest or what? #1798

Closed
dankelley opened this issue Mar 24, 2021 · 8 comments
Closed

dontrun or donttest or what? #1798

dankelley opened this issue Mar 24, 2021 · 8 comments
Assignees
Labels
CRITICAL Demands attention within days

Comments

@dankelley
Copy link
Owner

dankelley commented Mar 24, 2021

We have quite a few spots in documentation where \donttest is used to prevent testing, because the test involves data not provided with oce.

I think all of these might fail now, because CRAN is testing things inside \donttest blocks. I say that because the note I got from CRAN about oce being on the verge of deletion from CRAN had its problem in a new link called "donttest". (See snapshot below. Those NOTEs were not why oce is in trouble ... it's the new link called 'donttest'.)

The question is how we are now meant to "hide" examples that will not work on a test machine, e.g. because they need data not provided with oce, or because they are too slow (since CRAN has a limit on execution time of examples).

I need to look into this as a high priority, because I think we have a lot of things within dontest blocks. Starting points might be as follows.

Snapshot of oce check page Screen Shot 2021-03-24 at 7 12 56 AM
@dankelley dankelley self-assigned this Mar 24, 2021
@dankelley dankelley added the CRITICAL Demands attention within days label Mar 24, 2021
@dankelley
Copy link
Owner Author

dankelley commented Mar 24, 2021

Based on section 2.1.1 of https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Writing-R-documentation-files, I am starting to think we should change every \donttest to \dontrun.

I am a bit worried that Hadley Wickham (r-lib/devtools#2216 (comment)) seems to suggest that doing so might invite CRAN rejection. Still, I think this is worth the risk ... at least then we'll know more about the system.

Relevant portion of docs

The following is from section 2.1.1 of https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Writing-R-documentation-files; note that it will be formatted poorly, so your best plan is to go to the stated URL.

\examples{…}
Examples of how to use the function. Code in this section is set in typewriter font without reformatting and is run by example() unless marked otherwise (see below).

Examples are not only useful for documentation purposes, but also provide test code used for diagnostic checking of R code. By default, text inside \examples{} will be displayed in the output of the help page and run by example() and by R CMD check. You can use \dontrun{} for text that should only be shown, but not run, and \dontshow{} for extra commands for testing that should not be shown to users, but will be run by example(). (Previously this was called \testonly, and that is still accepted.)

Text inside \dontrun{} is ‘verbatim’, but the other parts of the \examples section are R-like text.

For example,

x <- runif(10) # Shown and run.
\dontrun{plot(x)} # Only shown.
\dontshow{log(x)} # Only run.
Thus, example code not included in \dontrun must be executable! In addition, it should not use any system-specific features or require special facilities (such as Internet access or write permission to specific directories). Text included in \dontrun is indicated by comments in the processed help files: it need not be valid R code but the escapes must still be used for %, \ and unpaired braces as in other ‘verbatim’ text.

Example code must be capable of being run by example, which uses source. This means that it should not access stdin, e.g. to scan() data from the example file.

Data needed for making the examples executable can be obtained by random number generation (for example, x <- rnorm(100)), or by using standard data sets listed by data() (see ?data for more info).

Finally, there is \donttest, used (at the beginning of a separate line) to mark code that should be run by example() but not by R CMD check (by default: the option --run-donttest can be used). This should be needed only occasionally but can be used for code which might fail in circumstances that are hard to test for, for example in some locales. (Use e.g. capabilities() or nzchar(Sys.which("someprogram")) to test for features needed in the examples wherever possible, and you can also use try() or tryCatch(). Use interactive() to condition examples which need someone to interact with.) Note that code included in \donttest must be correct R code, and any packages used should be declared in the DESCRIPTION file. It is good practice to include a comment in the \donttest section explaining why it is needed.

Output from code between comments

IGNORE_RDIFF_BEGIN

IGNORE_RDIFF_END

is ignored when comparing check output to reference output (a -Ex.Rout.save file). This markup can also be used for scripts under tests.

@dankelley
Copy link
Owner Author

FYI, this is the number of places where we use \donttest.

git grep '\\donttest'|wc
      35      37     700

@dankelley
Copy link
Owner Author

@paleolimbot

I will be resubmitting oce to CRAN, likely this Friday, to get around a problem with the present version of "sf" on CRAN (#1795).

We found out about this "sf" problem because Ripley's machine (and maybe now other machines) are running \donttest{} blocks.

We use \donttest{} blocks to illustrate the use of non-provided datasets (or to avoid very slow tests) on 35 occasions within oce. I think I ought to convert each of these to \dontrun{} blocks. However, I am a bit worried, because Wickham has said (in another context ... see above) that using \dontrun{} might invite package rejection.

QUESTION: Do you think I ought to switch the \donttest{} to \dontrun{}?

@dankelley
Copy link
Owner Author

dankelley commented Mar 24, 2021

The amazingly helpful @hadley has clarified that \dontrun{} causing rejection is mainly a worry for new submissions. So I'm going to go ahead and change to \dontrun{} all instances where we use \donttest{} because of non-provided data, whilst retaining those that are about slow operations. I'll need to look at the instances one by one.

Below is a checklist, created with git grep -n '\\donttest'. I will tick off items as I address them, but I will not push changes until all of these are done, local checks work, and R-CMD-check works.

The following codes show what I've done:

  • "RETAIN" means leave the example as it is (since it will work for users, but is too slow for CRAN)
  • "REMOVE" means to remove the \donttest markup, because the code runs fairly quickly
  • "RUN" means switched to \dontrun since uses data not provided
  • "RUN2" means switched to \dontrun because uses data in ocedata package (we cannot rely on that being loaded, and we cannot depend on it because that is circular)

Checklist

  • adp.R:244:#'\donttest{ RETAIN
  • adp.R:1164:#'\donttest{ RETAIN
  • adv.R:59:#'\donttest{ RETAIN
  • argo.R:45:#'\donttest{ REMOVE
  • coastline.R:473:#'\donttest{ RETAIN
  • coastline.R:1447:#'\donttest{ RETAIN
  • ctd.R:2014:#'\donttest{ RUN
  • ctd.R:2021:#'\donttest{ RUN
  • ctd.R:2033:#'\donttest{ RUN
  • echosounder.R:537:#'\donttest{ REMOVE
  • geod.R:50:#'\donttest{ RETAIN
  • geod.R:280:#'\donttest{ RETAIN
  • lobo.R:82:#'\donttest{ REMOVE
  • map.R:247:#'\donttest{ RETAIN
  • map.R:483:#'\donttest{ RETAIN
  • map.R:681:#'\donttest{ RUN2
  • map.R:878:#'\donttest{ RUN2
  • map.R:945:#'\donttest{ RUN
  • map.R:1019:#'\donttest{ RETAIN
  • map.R:1430:#'\donttest{ RETAIN
  • map.R:2100:#'\donttest{ RETAIN
  • map.R:2393:#'\donttest{ RETAIN
  • map.R:2494:#'\donttest{ RETAIN
  • map.R:2561:#'\donttest{ RETAIN
  • map.R:2618:#'\donttest{ RETAIN
  • map.R:2693:#'\donttest{ RETAIN
  • map.R:2763:#'\donttest{ RETAIN
  • map.R:3058:#' \donttest{ RETAIN
  • map.R:3624:#'\donttest{ REMOVE
  • map.R:3720:#'\donttest{ REMOVE
  • misc.R:919:#'\donttest{ RETAIN
  • misc.R:4353:#'\donttest{ RETAIN
  • misc.R:4376:#'\donttest{ RETAIN
  • sealevel.R:83:#'\donttest{ RETAIN

@paleolimbot
Copy link

Sounds like you have it under control! Another strategy you can use is wrapping examples conditionally like:

if (some_test) {
  # do the example
}

Mostly this is useful because then pkgdown will run your examples if some_test evaluates to TRUE on pkgdown or you can somehow set up your own system to ensure that the examples do in fact run. Useful values for some_test might be packageVersion("some.pkg") >= "some version", or curl::has_internet(). Just a thought!

@dankelley
Copy link
Owner Author

From my reading of the docs (section 2.1.1 of https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Writing-R-documentation-files) and my understanding of CRAN policies (which give a NOTE on slow examples), it seems to me that I ought to retain \donttest for slow-running examples. Do you think that's right, @paleolimbot?

@paleolimbot
Copy link

I think so. In addition to CRAN, it's useful in real life because \dontrun examples tend to slip through the cracks and become broken.

dankelley added a commit that referenced this issue Mar 24, 2021
See #1798, which was inspired by
a problem that was found within a donttest block
(#1795) and thus indicated that
CRAN is now running code within donttest blocks.  So, we ought to use
those blocks only for slow code that we know works.  For code that won't
work, e.g. because of the assumption of data not provided with the
package, we should use dontrun.
@dankelley
Copy link
Owner Author

dankelley commented Mar 24, 2021

I think this is fine now in "develop" commit fb18a76, as evinced by

  • local (macos) build and check
  • local (macos) R CMD check --as-cran on the tarball
  • remote R-CMD-check, which tests windows-latest (release), macOS-latest (release), ubuntu-20.04 (release), and ubuntu-20.04 (devel).
  • devtools:::check_win_devel()
  • devtools:::check_win_release()
  • devtools:::check_win_oldrelease(). This gives a NOTE about a problem with https://pubs.usgs.gov/pp/1395/report.pdf, perhaps because that file tends to download slowly. But the link works, so I am not inclined to change the \url{} to a \code{} unless I find a problem on Friday, which is the planned CRAN submission time.

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

No branches or pull requests

2 participants