About loading packages in tests about easystats HOT 11 CLOSED

I completely agree.

We should just use ::-syntax to access functions from suggested dependencies used for testing, and avoid attaching packages to search path.

That's going to be a lot of work, though, but worth it in the long run.

from easystats.

Nope, it's not enough:

library(testthat)

test_that("foo 1", {
  print("as.matrix.get_predicted" %in% methods(as.matrix))
})
#> [1] FALSE
#> ── Skip (???): foo 1 ───────────────────────────────────────────────────────────
#> Reason: empty test

test_that("foo 2", {
  withr::local_package("insight")
})
#> ── Skip (???): foo 2 ───────────────────────────────────────────────────────────
#> Reason: empty test

test_that("foo 3", {
  print("as.matrix.get_predicted" %in% methods(as.matrix))
})
#> [1] TRUE
#> ── Skip (???): foo 3 ───────────────────────────────────────────────────────────
#> Reason: empty test

withr::local_package() calls detach() under the hood, and the docs of detach() say:

If a package has a namespace, detaching it does not by default unload the namespace (and may not even with unload = TRUE)

I think we're doing the best we can about this

from easystats.

Using :: will probably solve the problem for functions exported by other packages but methods will still be available in the namespace so it won't solve everything (but I'm actually not sure we can do something to solve everything).

Also, this basically means reverting skip_if_not_or_load_if_installed() to skip_if_not_installed() + ::. Is there an easy way to revert some commits, even made a few days/weeks ago?

from easystats.

You can right click on the commit in the history in the GitHub desktop app (and maybe web interface) and choose to revert it.

git revert is the equivalent terminal command I think

from easystats.

I have cleaned up tests in {see} to address this issue (easystats/see#280).

from easystats.

Do we need to change the behavior of skip_if_not_or_load_if_installed() (cf #350) because it calls library()?

Actually, we should just get rid of that helper and just go back skip_if_not_installed().

Note that #350 was created in a different context. Most of our tests don't use :: to access functions from soft deps, and also don't explicitly load datasets from soft deps with data(), and so it was necessary to use require() calls. Additionally, we also wanted to reduce noise in tests due to loading these different packages, and so skip_if_not_or_load_if_installed() seemed like a good solution at the time.

If we want to use only skip_if_not_installed(), then we need to make sure all used functions are namespaced and datasets are explicitly loaded. That's why I said this is going to be a lot of work.

from easystats.

If we adopt this discipline, clutter in unit test logs (e.g.) would also disappear.

from easystats.

If we absolutely must load some packages in a test file, it's a good idea to unload the namespace at the end of the test file (cf. easystats/parameters@3912175).

from easystats.

I find that using namespacing instead of loading for rstanarm does not work most of the time. It's really funky. It tries to call functions that the script is not calling, but cannot find them. So they are used internally. Even defining those functions manually like stan_glmer <- rstanarm::stan_glmer results in more errors like:

Unable to refit the model with standardized data.
 Try instead to standardize the data (standardize(data)) and refit the
 model manually.

So I am loading it for those instances. But I wonder if someone found a way to make it work with namespacing somehow?

(same for survival package and Surv <- survival::Surv)

from easystats.

In the section on self-contained tests, R Packages book recommends using withr::local_package() to solve this issue.

But, IINM, @etiennebacher observed that that doesn't always work, right?

from easystats.

I think we have done everything we can here and there is not much to do.

from easystats.