Test conditionally on 'forecast' package - otherwise check error

[HenrikBengtsson]

I think your

origami/tests/testthat/test-overall_timeseries.R

Line 1 in a53ccfa

library(forecast)

needs to be using:
```r
if (require("forecast")) {
   ...
}

instead, because the 'forecast' package is optional (listed only in Suggests). Without the package installed, you get:

checking tests ...

 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  Version: 1.0.0
  > library(data.table)
  > test_check("origami")
  ── 1. Error: (unknown) (@test-overall_timeseries.R#1)  ─────────────────────────
  there is no package called 'forecast'
  1: library(forecast) at testthat/test-overall_timeseries.R:1
  2: stop(txt, domain = NA)
  
  ══ testthat results  ═══════════════════════════════════════════════════════════
  OK: 24 SKIPPED: 0 FAILED: 1
  1. Error: (unknown) (@test-overall_timeseries.R#1) 
  
  Error: testthat unit tests failed
  Execution halted
  Error while shutting down parallel: unable to terminate some child processes
checking re-building of vignette outputs ... WARNING

Error in re-building vignettes:
  ...
Warning in engine$weave(file, quiet = quiet, encoding = enc) :
  Pandoc (>= 1.12.3) and/or pandoc-citeproc not available. Falling back to R Markdown v1.
Quitting from lines 286-321 (generalizedCV.Rmd) 
Error: processing vignette 'generalizedCV.Rmd' failed with diagnostics:
there is no package called 'forecast'
Execution halted
checking package dependencies ... NOTE

Package suggested but not available for checking: ‘forecast’

[jeremyrcoyle]

Thanks for submitting this. I was under the impression that it was standard practice to install all the Suggests packages before running tests. Is that not the case? If not, I have a lot of other tests to fix as well :).

[HenrikBengtsson]

Although it's common practice it's not required. When Suggests:ed packages are missing, R CMD check should pass with only a NOTE (unless using _R_CHECK_FORCE_SUGGESTS_=TRUE in case it becomes an ERROR), e.g.

checking package dependencies ... NOTE

Package suggested but not available for checking: ‘forecast’

but they should never give an ERROR in the tests.

We use the same rationale when there's a Suggests:ed package that is only available on a single OS - then we test the code that depends on that conditionally on that OS. We would never accept tests to fail for the non-supported OSes.

There's been several discussion about this in the past, e.g. https://stat.ethz.ch/pipermail/r-devel/2016-April/072548.html. I like this thought:

The ideal situation would be to be able to run all possible combinations of missing Suggested packages, but that's probably far too slow to be a default.

which is also why it's not really enforced - it would be tricky for CRAN to test packages this way. However, I wouldn't be surprised if they one day check for this using static code inspection.

I also argue that it's better if each package developer is the gate keeper for their code, including their tests, to work with and without Suggests:ed packages. Otherwise, it'll be really complicated to do reverse package dependency tests (which is how I found yours) - because some Suggests:ed dependencies may be nested deep down.

[nhejazi]

Thanks for pointing this out @HenrikBengtsson. It's been fixed by 5e2ca19 and has been resubmitted to CRAN.