search

ropensci / vcr

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

set vcr_test_path() relative to testthat dir #243

Closed dpprdan closed 2 years ago

dpprdan commented 2 years ago

vcr_test_path() now looks for the testthat dir instead of tests, because the latter may not be present during R CMD check. fixtures (or however the folder is called by the user) is then put on the same level as testthat (instead of one level up from tests).

fixes #242 (hopefully)

maelle commented 2 years ago

This sounds like a great idea! Should the docs state this will only work with testthat? I'm not sure there are vcr users who don't use testthat, though.

dpprdan commented 2 years ago

Should the docs state this will only work with testthat?

I'd say the docs are pretty clear that vcr works only with assumes tests are done with testthat, see e.g. the first sentence in the Getting Started section. OTOH it doesn't hurt to mention it explicitly.

dpprdan commented 2 years ago

I just noticed that the README/Getting Started vignette still use

invisible(vcr::vcr_configure(
  dir = "../fixtures"
))

i.e. "../fixtures" to specify the fixtures directory instead of dir = vcr::vcr_test_path("fixtures") as mentioned in the http-testing book.

Should we change that?

dpprdan commented 2 years ago

BTW, if I am right about #244, or rather that #225 wasn't caused by vcr_test_path() - and that's a big IF - then we might be able to use a testthat::test_path() based approach after all and drop the rprojroot dependency. (IIUC we only began fiddling with vcr_test_path() because of #225).

This is based on multiple hunches though, so I'd go with the rprojroot::find_testthat_root_file() for now. We will have to revisit this topic again anyway, when we want to adapt vcr for other test suites (#162).

sckott commented 2 years ago

looks good to me. I don't use vcr_test_path so if others could please make sure to test this in their packages.