RGF-team / rgf

Home repository for the Regularized Greedy Forest (RGF) library. It includes original implementation from the paper and multithreaded one written in C++, along with various language-specific wrappers.
378 stars 58 forks source link

increase code coverage for the R-package #313

Closed mlampros closed 4 years ago

mlampros commented 4 years ago

I've added code coverage as discussed in #269. The only exception is the cleanup() method (either for a single estimator or for all estimators) which is tested (currently) on a unix-like OS. Feel free to close #269 if this PR works as expected.

One think that I've noticed is that the 'save_model' and 'dump_model' methods work only for RGF and not for FastRGF. thanks.

StrikerRUS commented 4 years ago

@mlampros Thank you very much for this PR!

Seems that two of new tests don't work on macOS:

* checking tests ...
  Running ‘testthat.R’ [22s/22s]
 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  > 
  > test_check("RGF")
  ── 1. Failure: the 'cleanup' method (ESTIMATOR specific) works as expected for b
  init_exists_upd_rgf && end_state_rgf && end_state_fastrgf isn't true.

  ── 2. Failure: the 'cleanup' method (APPLIES TO ALL ESTIMATORS) works as expecte
  &&... isn't true.

  ══ testthat results  ═══════════════════════════════════════════════════════════
  [ OK: 13 | SKIPPED: 0 | WARNINGS: 0 | FAILED: 2 ]
  1. Failure: the 'cleanup' method (ESTIMATOR specific) works as expected for both RGF and FastRGF (checking of the length of the '/tmp/rgf' default directory before and after the '$fit' method) (@test-RGF_package.R#501) 
  2. Failure: the 'cleanup' method (APPLIES TO ALL ESTIMATORS) works as expected for both RGF and FastRGF (checking of the length of the '/tmp/rgf' default directory before and after the '$fit' method) (@test-RGF_package.R#535) 

  Error: testthat unit tests failed
  Execution halted
mlampros commented 4 years ago

@StrikerRUS, I assumed that Mac osx will have the same temporary folder as on linux ('/tmp'). Is it ok if I restrict the cleanup() test only to the linux os? The cleanup() function is already tested on Python, and the R package doesn't do anything else than calling the python function from within R.

StrikerRUS commented 4 years ago

@mlampros I guess you can use tempdir() function as well as some environment variables to locate temp directory in a cross-platform way and don't hardcode it.

mlampros commented 4 years ago

@StrikerRUS, thanks for make me aware of the one-liner dirname(tempdir()), I think it will do the work. If not then I'll use the accepted answer of the second web-link that you've mentioned.

fukatani commented 4 years ago

@mlampros Sorry for late response. Are you debugging failure on appveyor?

mlampros commented 4 years ago

@fukatani, I'll add the remaining tests for this PR tonight. I think, the appveyor error is a connection failure, so with a re-run it will disappear. thanks.