Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-consider where helper files are sourced in load_all() #1146

Closed
berndbischl opened this issue Apr 14, 2016 · 17 comments
Closed

Re-consider where helper files are sourced in load_all() #1146

berndbischl opened this issue Apr 14, 2016 · 17 comments
Labels
Milestone

Comments

@berndbischl
Copy link

@berndbischl berndbischl commented Apr 14, 2016

Your NEWS state this

load_all() now sources all test helpers if you use testthat. This makes it much easier to interactively run tests (#1125). load_all() also correctly handles unix and windows subdirectories within R (@gaborcsardi, #1102)

In my setup I have a helper.R file, in tests/testthat which creates a few (data) objects I want to use in my unit tests (among other stuff).

You can reproduce the problem by creating a basically empty (but valid) package, then adding such a file under tests/testthat

data(Sonar, package = "mlbench")
x = Sonar

This now breaks, after the new devtools update

> load_all()
Loading minipkg
Error in eval(expr, envir, enclos) : object 'Sonar' not found

mlbench would usually be in my SUGGESTS (and I want it to be there) but even adding it to DEPENDS and adding and explicit call to library("mlbench") in the helper.R does not remove the problem.

This now breaks our complete devel workflow.

Any advice?

@jimhester
Copy link
Member

@jimhester jimhester commented Apr 14, 2016

The problem is the helpers are sourced in the package environment, but data() by default loads the objects in the global environment. Explicitly loading into the calling environment fixes the issue. (see mlr-org/mlr#835)

@hadley maybe we should add a data() shim which makes this the default?

@jimhester jimhester reopened this Apr 14, 2016
@jimhester jimhester changed the title devtools 1.11.0 looad_all breaks if data is called in testthat helper files devtools 1.11.0 load_all breaks if data is called in testthat helper files Apr 14, 2016
larskotthoff added a commit to mlr-org/mlr that referenced this issue Apr 14, 2016
@berndbischl
Copy link
Author

@berndbischl berndbischl commented Apr 14, 2016

Explicitly loading into the calling environment fixes the issue.

Cool that you are even fixing our package for us :) Thx a lot!

@hadley maybe we should add a data() shim which makes this the default?

I would seriously suggest that. What happened on our side was that I wanted to show a student assistant how the mlr unit tests work. Apparently at that point in time we got the new devtools version on her laptop. After calling load_all on the package, and seeing the error, we had 3 technically competent people sitting around the laptop for 30 min thinking "wtf is happening".
The error is extremely "unintuitive" and very hard to debug. I basically had to guess what happened, and downgrade devtools to figure this out.

@imanuelcostigan
Copy link
Contributor

@imanuelcostigan imanuelcostigan commented Apr 16, 2016

I have a package that creates some option settings through the .onAttach hook. My test helper files require these options. Unfortunately, devtools runs the package hooks after sourcing the test helpers and breaks things for me. Why can't the test helper files be loaded after the package is attached by load_all()?

@imanuelcostigan
Copy link
Contributor

@imanuelcostigan imanuelcostigan commented May 1, 2016

@hadley would you be open to me submitting a PR with the effect of sourcing the test helper files after running hooks?

PS - check() passes without error after making this change.

@hadley
Copy link
Member

@hadley hadley commented May 1, 2016

Sure

@berndbischl
Copy link
Author

@berndbischl berndbischl commented May 1, 2016

one other question from my side, that i just noticed.

in the past, i also created a file called helper_zzz.R
this eg set a seed for my unit tests. and this seed should ONLY be set for the unit tests.
now it is always used (when i call load_all on the package)

so i guess my question is:
how can i do "state changes" in my helper files, which are only executed when i am in "testthat unit testing mode" ?

@hadley hadley added this to the 1.12.0 milestone Jun 2, 2016
@hadley hadley changed the title devtools 1.11.0 load_all breaks if data is called in testthat helper files Re-consider where helper files are sourced in load_all() Jun 3, 2016
@hadley
Copy link
Member

@hadley hadley commented Jun 3, 2016

@berndbischl maybe condition on interactive()?

@jennybc
Copy link
Member

@jennybc jennybc commented Jun 3, 2016

It feels like part of the pain around test helpers is that they are used for setup but there's no similarly official way to do corresponding tear down. I have a faux test file named test-zz-clean-up.R that does this. Is that awful?

@hadley
Copy link
Member

@hadley hadley commented Jun 3, 2016

What are you typically tearing down?

@jennybc
Copy link
Member

@jennybc jennybc commented Jun 3, 2016

OK this will be very specific. But I remove an OAuth token from an environment internal to the package. I delete some local files. I delete some things from Google Drive.

@hadley
Copy link
Member

@hadley hadley commented Jun 3, 2016

Maybe what we need for the non-idempotent stuff is to simply source a setup.R before running the tests and a teardown.R after the tests?

@jennybc
Copy link
Member

@jennybc jennybc commented Jun 3, 2016

Is there something like on.exit() in this context?

@hadley
Copy link
Member

@hadley hadley commented Jun 3, 2016

No, but that would be another approach - if you wanted to keep setup and tear down in the same file together. Could have something that allowed you to register setup and teardown code for each run.

I'm thinking something like:

register_test_something(
  setup = file.create("x"),
  teardown = unlink("x")
)
@berndbischl
Copy link
Author

@berndbischl berndbischl commented Jun 4, 2016

@berndbischl maybe condition on interactive()?

we also run our unit tests in non-interactive mode via shell scripts / from the console. actually we do that most often.

@pitakakariki
Copy link

@pitakakariki pitakakariki commented Jun 7, 2016

@berndbischl maybe condition on interactive()?

I think interactive() returns TRUE whether you've called load_all() or test().

@hadley
Copy link
Member

@hadley hadley commented Aug 2, 2017

Moved to pkgload

@hadley hadley closed this Aug 2, 2017
@lock
Copy link

@lock lock bot commented Sep 18, 2018

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Sep 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants