Can't use `load_all(export_all = FALSE)` for dbplyr
mgirlich opened this issue · 3 comments
Not sure if this is a dbplyr or a pkgload issue. The variable test_srcs is defined in R/test-frame.R
pkgload::load_all("~/GitHub/dbplyr/", reset = TRUE, export_all = TRUE)
#> ℹ Loading dbplyr
#> Registering testing src: sqlite
#> OK
#>
#> Registering testing src: MariaDB
#> OK
#>
#> Registering testing src: postgres
#>
#> * connection to server at "localhost" (127.0.0.1), port 5432 failed: Connection refused
#> Is the server running on that host and accepting TCP/IP connections?
#> connection to server at "localhost" (::1), port 5432 failed: Connection refused
#> Is the server running on that host and accepting TCP/IP connections?
pkgload::load_all("~/GitHub/dbplyr/", reset = TRUE, export_all = FALSE)
#> ℹ Loading dbplyr
#> Error in `source_file()`:
#> ! In path: "/Users/mgirlich/GitHub/dbplyr/tests/testthat/helper-src.R"
#> Caused by error:
#> ! object 'test_srcs' not found
#> Backtrace:
#> ▆
#> 1. ├─pkgload::load_all("~/GitHub/dbplyr/", reset = TRUE, export_all = FALSE)
#> 2. │ └─pkgload:::populate_pkg_env(...)
#> 3. │ ├─withr (local) withr_with_envvar(...)
#> 4. │ │ └─base::force(code)
#> 5. │ └─testthat (local) testthat_source_test_helpers(find_test_dir(path), env = pkg_env)
#> 6. │ └─testthat::source_dir(path, "^helper.*\\.[rR]$", env = env, wrap = FALSE)
#> 7. │ └─base::lapply(...)
#> 8. │ └─testthat (local) FUN(X[[i]], ...)
#> 9. │ └─testthat::source_file(path, env = env, chdir = chdir, wrap = wrap)
#> 10. │ ├─base::withCallingHandlers(...)
#> 11. │ └─base::eval(exprs, env)
#> 12. │ └─base::eval(exprs, env)
#> 13. └─base::.handleSimpleError(...) at tests/testthat/helper-src.R:4:0
#> 14. └─testthat (local) h(simpleError(msg, call))
#> 15. └─rlang::abort(...)Created on 2023-06-07 with reprex v2.0.2
The problem is that the helper files are sourced (via testthat::source_test_helpers()) into the environment created by load_all(). When export_all = FALSE, the helpers can't access the unexported bindings.
I'm not sure how we could do better here. Perhaps we should source the helpers into the namespace rather than the package env? This means that from the perspective of devtools, testthat helpers would effectively be part of the namespace (and overwrite anything in there). However that still wouldn't interact well with export_all = FALSE as the package env would no longer have the helpers sourced in.
Another way that might work would be to start the package env as a full clone of the namespace, and then remove non-exported bindings. But that would be kind of fiddly. We'd have to be able to distinguish between bindings added by helper files (that should be kept in) and the other non-exported bindings, both those created statically (in the R/ files) and dynamically (from .onLoad()). Actually I'm not sure how we can do this.
Why do you need to use export_all = FALSE? It feels like this argument makes pkgload too complex, I wonder if we should consider deprecating it.
Or perhaps helpers = should default to the value of export_all. If both parameters are set to the same value, that'd always be ok. If the user wants to export everything except the helpers, ok as well. But trying to export the helpers and not the internal tools would be an error. I'm now leaning towards that option. Does that make sense @hadley?
I don't need to use it directly, rather I wanted to execute devtools::run_examples() in the dbplyr package and this failed. I boiled this down to load_all(export_all = FALSE). Sorry, forgot to mention that in the issue description. Maybe you want to fix this in run_examples() instead.
Thanks for the context. It seems like changing the default to helpers = export_all so that helpers are not sourced when extra exports are disabled would solve this particular problem. If we don't feel comfortable making export_all = FALSE, helpers = TRUE an error, this would be enough to close this issue.