Set reproducibility options in test_that()

[hadley]

Fixes #1044. Fixes #971.

[jennybc]

My main meta thought is: what if you have additional test config that is unique to your package? But applies broadly across your tests.

Is there any way to register your own options or env vars to set and restore?

In usethis, for example, that would potentially apply to options(usethis.quiet = TRUE).

[gaborcsardi]

Looks great!

One thing to think about is whether we want to handle this from the other end. I.e. should cli notice that it is running in testthat, and then modify its output accordingly?

Alternatively, should we support a hook, so packages can add their own options or callbacks that are set or called by test_that()? We would do this in addition to the currently set options and env vars. We don't need to do this in this PR, but in the long run it could be a more sustainable solution I think.

EDIT: I can see now that @jennybc brought up the same thing. :)

[gaborcsardi]

Is there any way to register your own options or env vars to set and restore?

testthat cannot provide an API for this, because you would not want to import testthat. So yeah, one solution would be to set options with a specific name, and a setup and a restore function. E.g. usethis would do something like this in .onLoad():

options(
  testthat_context_usethis = list(
    setup = function(.env) {
      options(usethis.quiet = TRUE)
    },
    restore = function(.env, old) {
      options(old)
    }
  )
)

testthat would get all the testthat_context_* options, and call their setup() functions at the beginning of test_that() and then call their restore() functions at the end, passing the return value of setup() to the latter, in addition to the env.

[hadley]

Packages can already uses the TESTTHAT env var to condition behaviour based on whether or not testthat is running. Do we need something more?

[jennybc]

Packages can already uses the TESTTHAT env var to condition behaviour based on whether or not testthat is running. Do we need something more?

I guess it's a question of where to put the instructions to do THIS not THAT when testing.

If there were a way to register package-specific test state instructions, then they are all centralized in, e.g., onLoad(), following @gaborcsardi's proposal.

If we're supposed to consult the env varTESTTHAT, these conditionals potentially end up sprinkled in function bodies throughout the package.

I feel like centralizing them makes them much more visible and maintainable.

[hadley]

I don't see how we can make a registry work when you are running the tests interactively. That would force you to also run local_test_context() so that the behaviour matches up inside and outside of the test (i.e. in more cases than you do now).
Overall, making this hookable at the package level, feels to me like a step backward; we generally want to reduce the amount of implicit context in a test, not increase it.

[gaborcsardi]

Packages can already uses the TESTTHAT env var to condition behaviour based on whether or not testthat is running. Do we need something more?

Maybe. For the other package, I think just setting an option is easier. But we don't have to rush this now I think.

[gaborcsardi]

I don't see how we can make a registry work when you are running the tests interactively. That would force you to also run local_test_context() so that the behaviour matches up inside and outside of the test (i.e. in more cases than you do now).

Yes, when you run parts of a test_that() block interactively, you already have to run local_text_context() I believe. No?

Overall, making this hookable at the package level, feels to me like a step backward; we generally want to reduce the amount of implicit context in a test, not increase it.

I agree that using this well needs some discipline from the 3rd party packages. But if we don't have a hook, then we need to collect all such information in testthat, which also does not seem right. (Although the TESTTHAT env var is already a hook as you say.)

[gaborcsardi]

I feel like centralizing them makes them much more visible and maintainable.

Yeah, I agree. The alternative is to introduce a helper function into your package (usethis), that queries TESTTHAT first, and only consults the option (usethis.quiet) if it is not set. That is not too bad, either, I think.

E.g cli already has this for console with, so I'll only need to update the console_width() function to check for TESTTHAT. usethis could get an is_quiet() function I guess.

[hadley]

Hmmm I just noticed that rlang::is_interactive() already checks for the testthat envvar, so I don't need to add it here.

[gaborcsardi]

Yeah, I think just having a helper that checks for TESTTHAT before the user-defined option is fine and much simpler.

[hadley]

One more point in favour of doing this in one place — it's nice to know that you can load at local_test_context() to see all the options that are set, rather than having to carefully read the docs for all the functions that you call

[gaborcsardi]

Yeah, we want to have it in one place as well, I think. That's actually the motivation for the API in #1054 (comment)

With that, for a given test_that() block, you can list all options/env vars/etc. that are changed, by simply looking at options().

If packages will change their behavior based on TESTTHAT, then you cannot really do this.

So in this respect the API is better I think. But I am not entirely convinced if it is needed, or it is too much.