ropensci / vcr

Record and replay HTTP requests
https://docs.ropensci.org/vcr
Other
77 stars 12 forks source link

vcr not compatible with tools::testInstalledPackage #248

Closed maksymiuks closed 2 years ago

maksymiuks commented 2 years ago

Hi,

recently I've found out that packages using vcr configuration for tests, cannot be tested using tools::testInstalledPackage if the value of the outDir does not target the package's source. The problems come to from the vcr_test_path function, in particular call to the rprojroot::find_testthat_root_file function here https://github.com/ropensci/vcr/blob/003adc25a24ffc4eb312e5046c797d1e7009e12c/R/vcr_test_path.R#L23. While running tests, tools::testInstalledPackage function copies tests directory to outDir and renames it to package_name-tests. That's why rprojroot::find_testthat_root_file is unable to identify tests directory so the function crashes and thus all the tests. I'd consider changing the function used to find the root and account for the base mechanism which tools::testInstalledPackage is. I think that looking for tests directory or matching -tests phrase should be enough.

If you agree with my rationalization and need any help with changing that one, I'm happy to help.

Best Szymon

sckott commented 2 years ago

Thanks for pointing this out @maksymiuks

@maelle @dpprdan looping you two in since you were involved with this fxn. What do you think of the proposal?

dpprdan commented 2 years ago

Thanks for reporting @maksymiuks!

(Please add a reproducible example the next time you submit an issue or state why it is not possible to provide one. A reproducible example helps us tremendously zooming in on the problem.).

vcr_test_path() assumes that you are using testthat and that tests are located in the tests/testthat directory below the package root, accordingly. However, testInstalledPackage() does not work with any package that uses testthat, when they are installed per default.

The reason is this: testInstalledPackage() first looks up the package path and then checks, whether a tests directory is present in that directory. However, the tests dir is usually not present for an installed package, if that package uses {testthat} or rather if it has it's test in any directory other than inst/tests in its package source, and if it has been installed the default way. In fact, of the almost 400 packages in my personal library, only 6 have a tests directory in the package root directory (e.g. {data.table}, {Hmisc}, {yaml} or {whisker}). You can check if the tests directory is available for an installed package with system.file("tests", package = "yourPackage") - the tests dir exists if the returned path is not empty.

It is possible to also install the tests from the package (dev) root. From the testInstalledPackage() help:

If package-specific tests are found in a ‘tests’ directory they can be tested: these are not installed by default, but will be if R CMD INSTALL --install-tests was used.

I've checked this with the exemplighratia test package:

  1. Build source package (make sure to have the vcrtest branch of exemplighratia checked out!)
  2. Install with R CMD INSTALL exemplighratia --install-tests
  3. Run tools::testInstalledPackage("exemplighratia", types = "tests")
# `tests` dir exists in the installed package root
dir.exists(system.file("tests", package = "exemplighratia"))
#> [1] TRUE
# which test files are present
dir(system.file("tests", package = "exemplighratia"))
#> [1] "fixtures"   "testthat"   "testthat.R"
# run testInstalledPackage()
tools::testInstalledPackage("exemplighratia", types = "tests")
#> Running specific tests for package 'exemplighratia'
#>   Running 'testthat.R'

So testInstalledPackage() works like it should with vcr_test_path() 🎉

Summing up, it's not a {vcr} bug that testInstalledPackage() does not work for most packages which use {testthat} (I posit that it's not even a {testthat} bug either, but due to the fact that R does not install the tests dir by default). However, if the tests dir is present after installing with the --install-tests flag, then {vcr} and vcr_test_path() works as expected.

maksymiuks commented 2 years ago

Thanks for the comprehensive answer!

Now I see why I could've prepared an example package to illustrate my issue. All you said is 100% right however it was not my point. You are mentioning the situation when tests could not be installed and thus testInstalledPackage() which is obviously expected and thoroughly presented by you. What I wanted to do is to emphasize a problem where the package was installed correctly with tests but still the correct path was not found. Here is my example

install.packages("vcr")
remotes::install_github("maksymiuks/testpkg", INSTALL_opts = "--install-tests")
tools::testInstalledPackage("testpkg", outDir = tempdir(), types = "tests")
cat(readLines(file.path(tempdir(), "testpkg-tests", "testthat.Rout.fail")))

as you see we are getting an error

Error: No root directory found in /private/var/folders/45/szy8wtqj18g3x4mw1khk5jnr0000gn/T/RtmpdKs1ty/testpkg-tests/testthat or its parent directories. Root criterion: contains a directory "tests" Execution halted

and this was my suggestion for this fix.


While finalizing this post I realized that this issue is already sorted in the dev version here on github. So the problem was caused by the rprojroot::has_dir("tests") call that is still in the CRAN version, rprojroot::find_testthat_root_file function works just fine and is robust against testInstalledPacakge(), I'm deeply sorry for the confusion! However, I have to ask, when do you expect to release a new version on CRAN?

dpprdan commented 2 years ago

However, I have to ask, when do you expect to release a new version on CRAN?

That's up to @sckott

sckott commented 2 years ago

Thanks so much @dpprdan for your detailed help here.

Sorry for the delay. This is the milestone lined up for the next cran version https://github.com/ropensci/vcr/milestone/8

The hardest bit is #245 - although it lacks details, it's a huge PITA related to an upcoming R version, trying to track down encoding issues. I just haven't had time to deal with it. I don't know when I'll push the next one. I'd guess the end of October.

sckott commented 2 years ago

I think we're all set here? can this be closed?