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

tinytest and Rcpp (Attributes) are not playing along #37

Closed
eddelbuettel opened this issue Sep 14, 2019 · 13 comments
Closed

tinytest and Rcpp (Attributes) are not playing along #37

eddelbuettel opened this issue Sep 14, 2019 · 13 comments
Labels
bug Something isn't working duplicate This issue or pull request already exists

Comments

@eddelbuettel
Copy link
Contributor

My plan of switching the Rcpp family to tinytest hit the skids on the first test I tried for RcppArmadillo.

In a nutshell, an existing test file like this one for RUnit has a call to on-demand compile a C++ file with the C++ side of the tests. We use an in-package helper function for that in Rcpp and RcppArmadillo, but mostly only because RUnit is not as clever as tinytest about where it runs.

So then for tinytest, I just tried

library(RcppArmadillo)

Rcpp::sourceCpp("cpp/complex.cpp")

## tests below

which "should just work" -- and does indeed on the command-line with either the file or directory runner:

edd@rob:~/git/rcpparmadillo(feature/tinytest)$ tt.r -f inst/tinytest/test_complex.R 
Running test_complex.R................   11 tests OK 
[1] "All ok, 11 results"
edd@rob:~/git/rcpparmadillo(feature/tinytest)$ tt.r -d inst/tinytest/
Running test_complex.R................   11 tests OK 
[1] "All ok, 11 results"
edd@rob:~/git/rcpparmadillo(feature/tinytest)$ 

But when I let R CMD check loose, all hope is lost, and I get an error message I do not think I have ever seen before (!!):

* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ...
  Running ‘doRUnit.R’
  Running ‘tinytest.R’
 ERROR
Running the tests in ‘tests/tinytest.R’ failed.
Last 13 lines of output:
  Calls: local ... eval.parent -> eval -> eval -> eval -> eval -> source -> file
  In addition: Warning message:
  In file(filename, "r", encoding = encoding) :
    cannot open file 'startup.Rs': No such file or directory
  Execution halted
  
  Error in Rcpp::sourceCpp("cpp/complex.cpp") : 
    Error 1 occurred building shared library.
  Calls: <Anonymous> ... run_test_dir -> lapply -> FUN -> eval -> eval -> <Anonymous>
  
  WARNING: The tools required to build C++ code for R were not found.
  
  Please install GNU development tools including a C++ compiler.
  
  Execution halted
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes in ‘inst/doc’ ... OK

No C++ compiler? Really? Could it be that tinytest set things up so tight that certain maintained assumptions no longer hold?

@eddelbuettel
Copy link
Contributor Author

FWIW same behaviour on 1.0.0 and 1.0.0.7.

@markvanderloo
Copy link
Owner

Ha..have a quick look at #36

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Sep 14, 2019

I had. But I suspect that is not it. Blows up with ncpu=NULL as well. And yes, I added that env.var (in the test file).

I did add that env.var in the tests/ runner for RUnit though before. Maybe I should try there.

@eddelbuettel
Copy link
Contributor Author

Tried again. No impact. Still fails. 😢

@eddelbuettel
Copy link
Contributor Author

Attached is a trimmed-down version of the package (no vignettes/, no RUnit use). It has the one test failing. Maybe you can dive into tinytest and see why running the test file 'directly' works but checking the package does not?

RcppArmadillo_0.9.700.2.0.1.tar.gz

@eddelbuettel
Copy link
Contributor Author

I think I got it. We need two things, and I had only done one at a time (doh)

  • your hunch is correct, one does need R_TESTS="" as an environment variable; that is an R constant
  • one the also needs to tell tinytest to not mess with environment variables, i.e. to not check for side effects

That seems to work. 🍰

Medium-term, I see two things:

  • you may as well set R_TESTS="" for everybody as a courtesy (and just document it)
  • if you don't want to do that then I think you need to tell the side effects checker routine to leave R_TESTS alone

@eddelbuettel
Copy link
Contributor Author

Here is a better version proving that theory (the .1 above had one other wart)

RcppArmadillo_0.9.700.2.0.2.tar.gz

@eddelbuettel
Copy link
Contributor Author

And parallel tests work fine too.

@markvanderloo
Copy link
Owner

Wow, thanks for all that work! I haven't had a chance this weekend to look into it. But this is something I can definitely work with.

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Sep 15, 2019

All works wonders for me if and only if I turn side-effects off in the runner in tests/. Else it fails

R_TESTS="" is in fact needed, so I think you should think hard about making it a default (so that users of Rcpp etc that need it benefit). I don't think there are side-effects, but we can test. AFAIK every unit test framework has fallen into this hole because R CMD check is so tight about the path.

Otherwise, tinytest is glorious. Look at this:

edd@rob:~$ tt.r -p RcppArmadillo    ## littler helper in 'test package' mode
starting worker pid=6475 on localhost:11311 at 13:17:36.362
starting worker pid=6502 on localhost:11311 at 13:17:36.509
starting worker pid=6543 on localhost:11311 at 13:17:36.650
starting worker pid=6568 on localhost:11311 at 13:17:36.790
Running test_rng.R....................   10 tests OK 
Running test_complex.R................   11 tests OK 
Running test_sample.R.................   21 tests OK 
Running test_cube.R...................   18 tests OK 
Running test_fastLm.R.................   25 tests OK 
Running test_rcpparmadillo.R..........   48 tests OK 
Running test_Rlapack.R................    7 tests OK 
Running test_scipy2r.R................    4 tests OK 1 side-effects
Running test_rmultinom.R..............    9 tests OK 
Running test_sparse.R.................   12 tests OK 
Running test_sparseConversion.R.......   86 tests OK 
----- SIDEFX[envv]: test_scipy2r.R<33--33>
 call| if (!py_module_available("scipy")) exit_file("Module scipy missing")
 diff| Added   envvar 'R_SESSION_INITIALIZED' with value 'PID=6568:NAME="reticulate"'
 diff| Added   envvar 'RETICULATE_REQUIRED_MODULE' with value 'scipy'
 diff| Changed envvar 'PATH' from '/home/edd/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin' to '/usr/bin:/home/edd/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin'
 
Showing 1 out of 252 results: 0 fails, 251 passes, 1 side effects
edd@rob:~$
edd@rob:~$
edd@rob:~$ tt.r -pz RcppArmadillo  ## littler helper in 'test package' mode, suppress fx
starting worker pid=7234 on localhost:11807 at 13:17:43.703
starting worker pid=7261 on localhost:11807 at 13:17:43.846
starting worker pid=7301 on localhost:11807 at 13:17:43.987
starting worker pid=7327 on localhost:11807 at 13:17:44.132
Running test_complex.R................   11 tests OK 
Running test_rng.R....................   10 tests OK 
Running test_sample.R.................   21 tests OK 
Running test_cube.R...................   18 tests OK 
Running test_fastLm.R.................   25 tests OK 
Running test_rcpparmadillo.R..........   48 tests OK 
Running test_Rlapack.R................    7 tests OK 
Running test_rmultinom.R..............    9 tests OK 
Running test_scipy2r.R................    4 tests OK 
Running test_sparse.R.................   12 tests OK 
Running test_sparseConversion.R.......   86 tests OK 
[1] "All ok, 251 results"
edd@rob:~$ 

And I can't even sheepishly try to 'undo' the env var settings that reticulate causes as you;d catch me :). This is truly fabulous stuff.

@markvanderloo
Copy link
Owner

Found this note by @HenrikBengtsson. Looks like currently, R_TESTS is only used to define a new startup file startup.Rs which currenlty only sets the useFancyQuotes option to FALSE. This is something that can easily be mocked without disturbing R CMD check, but we need to watch it for changes.

Also, I have to make sure this is done for all parallel sessions in case of parallel testing.

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Sep 17, 2019

Yes. See the grep / ag snippets I sent you. Both say the same thing. And as usual, what
@HenrikBengtsson says is of cause more useful and useable than my ramblings 😁

@markvanderloo
Copy link
Owner

Fixed in 0ab0659 and tested on RcppArmadillo. Also affects #36

@markvanderloo markvanderloo added bug Something isn't working duplicate This issue or pull request already exists labels Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants