rstudio / shinytest

Automated testing for shiny apps
https://rstudio.github.io/shinytest/
Other
225 stars 55 forks source link

`devtools::test()` requires *installed* package #397

Closed maxheld83 closed 3 years ago

maxheld83 commented 3 years ago

As per current instructions:

https://github.com/rstudio/shinytest/blob/8715e57a149a8350ea30bec5d3f010343414044e/vignettes/package.Rmd#L109-L113

I think a successful testthat::test_package() requires the package in question to be installed (via, e.g. remotes::install_local()).

Otherwise, for example:

Error (test-shiny.R:5:3): hello_world_app() works
Error: Error starting application:
Loading required package: shiny
Error in library(wama) : there is no package called ‘wama’
Backtrace:
  1. shinytest::expect_pass(...) test-shiny.R:5:2
  2. shinytest::testApp(test_path("apps/hello_world"), compareImages = FALSE)
  3. base::lapply(...)
  4. shinytest:::FUN(X[[i]], ...)
  5. base::source(testname, local = env)
  7. [ base::eval(...) ] with 1 more call
  9. ShinyDriver$new("../../")
 10. shinytest:::initialize(...)
 11. shinytest:::sd_initialize(...)
 12. private$startShiny(...)
 13. shinytest:::sd_startShiny(...)

This is somewhat counter to the usual testthat::test_*() logic, many of which seem to be running (and are happy with) devtools::load_all() under the hood.

I'm guessing this is because shinytest spins up a new process or something, where it doesn't have the same environment available that other tests have, and thus lacks the objects resulting from load_all().

maxheld83 commented 3 years ago

@schloerke mentions in https://github.com/rstudio/shinytest/pull/398 that just running devtools::load_all() in the background process may not be a great idea.

Any chance that alternatively, the background process may inherit the environment from the caller, when that caller is testthat::test_*()?

I see that this may all just be adding more headaches than it solves 😕.

I just noticed the different behavior between testthat and shinytest and I just know I'll forget about this, and then in a couple of months, I'll be so confused why some change I made fixes testthat-tests, but not the shinytest-tests (because I will have forgotten to remotes::install_local() 🙈).

schloerke commented 3 years ago

Agreed. It confuses me as well.

I don't think we'll change it here in shinytest. But maybe we could solve it in shinytest2 (private repo for now) in the coming month.

I originally was thinking of solving this via non-blocking shiny. (Allowing Shiny to be used in the foreground, rather than the background R process.) But this too far out.

Maybe we could check which packages are loaded via devtools and reload them on start of the background R process. 🤔 I know we can look for packages that have been loaded, but IDK where they were loaded from. Ex:

asNamespace("shinytest")$`.__DEVTOOLS__` %>% ls(envir = ., all.names = TRUE)
#> [1] ".onLoad"
asNamespace("webshot2")$`.__DEVTOOLS__`$.onLoad
#> [1] TRUE

If we can find that info, then I feel safer using devtools::load_all() when starting the R process.

maxheld83 commented 3 years ago

It occurred to me that my idea of baking devtools::load_all() into tests/testthat/apps/hello_world/app.R was also a stupid idea, because it actually creates another test-time dependency (whether devtools or, the slimmer pkgload). That just seems bad because we don't otherwise just use random packages (that are not Suggests:) in tests/.

So, bad idea on my part.

Closing this to reduce noise, with warning covered in #399.

maxheld83 commented 3 years ago

FWIW, if anyone else stumbles upon this, the mastering-shiny way of testing (you "manually" I/O the ShinyDriver object) is not affected by this. Because there's no background process, the usual usethis logic works.