Enhancement#305 gridded global datav5

[nip5]

Merge gridded 250m soil data extraction functionality.

[nip5]

Some new lintr tests will fail because of commit b28734c but all tests pass for my function.

Addressed

[dschlaep]

My review is commenting on the code, but does not include running the tests and check whether the code produces what we want. @CaitlinA can do that better and should give the final ok, once all else is resolved. Thanks.

[dschlaep]

These are non-standard tests because they require data outside the source package, i.e., they are not self-sufficient.

I think that this is ok in this case, but it requires substantial documentation at the top of this file for future developers, e.g., explain exactly what external data files are required and how to set up the paths correctly.

I found that your idea was good with the run_tests which was turned off by default. Unfortunately, you removed this switch. This switch prevented these tests to be run (and fail) unless a future developer knows exactly how to set up these specific tests.

You define paths and the code does not check whether or not these exists. The tests as they are currently will fail the paths and files don't exist, i.e., anytime these tests are run not on your correctly setup machine -- e.g., this fails on @CaitlinA 's or my machine. I suggest that these tests should skip with some appropriate message if the paths are not correctly set up and the necessary files do not exists.

[dschlaep]

Why do you test whether this has online access? I don't see anything in your tests that requires online access.

[nip5]

You asked me to model setting up whether or not to skip the tests on other tests, thus I removed the run_tests flag since no other test does it that way but I do agree that it was useful. The code being tested will tell you if the paths are wrong ie. if you run the tests with an incorrect soils extraction path if will tell you the folder or file does not exist. Are you suggesting that I check those in the tests before running them as well?

[dschlaep]

• I only referred to the use of skip* functions which are intended for use within test_that() blocks, i.e., they don't skip if outside a test_that block as you had it

• I didn't suggest at all that you test online access and I didn't say anything about removing run_tests

• If you don't skip your tests if the external files don't exist, then running devtools::test() will fail -- except on your specific machine. I think this is bad. You are introducing here a new type of tests to our package -- your tests are the first to assume data outside the source package and besides online accessible data. Thus, you need to handle your new test gracefully and differently such that other people's workflow is not being broken.

[nip5]

I understand the intended use of the skip functions but it sounds much more helpful to give a user a reason for skipping the test using skip() outside of testthat functions rather than giving no explanation when skipping by simply bypassing the code as is the case in tests such as test_net_CDF_function.R.

[dschlaep]

skipping the test using skip() outside of testthat functions

This doesn't work. Have you ever tried it out???

give a user a reason for skipping the test ... rather than giving no explanation

No one prevented you from providing a reason for skipping; it would be easy enough to add a if (!any(do_skip)) {} else {test_that("block name", {skip(your message)})}. Plus, a package user never sees the tests; it is only developers that see the tests; thus, the code for do_skipcalculation intest_netCDF_functions.R` provides a detailed explanation in the comments.

[dschlaep]

I still would like to see some explanation at the top of this file that explains the special nature of these tests including stating that data that are not included in the package are required and need to be prepared manually -- as it is, the next developer will waste time wading through your test code, has to figure out that she/he needs to manually set dir_ex_soil; will realize that the tests still fail; will have to dig further and realize that function extract_soil_ISRIC250m needs special folder hierarchy and files, etc.

Why not a few sentences documenting and explaining???

[dschlaep]

Why absolute paths for fslayers and fsoils specific to your computer? The folder TestPrj4 is part of this package source, thus, it will always be the same and you should use a relative path, e.g., see example in test_projects.R which uses dir_tests <- file.path("..", "test_data", "TestPrj4")

[dschlaep]

You don't check that dir_ex_soil contains the required content for function extract_soil_ISRIC250m. Let's assume that dir_ex_soil exists but the person has not downloaded the specific dataset, then this still fails because of

dir.ex.gridded <- file.path(dir_ex_soil, "ISRIC", "GriddedGlobalV5")
  # stop program execution if folder path is incorrect
  if (!dir.exists(dir.ex.gridded)) stop(paste0("Folder '", dir.ex.gridded,
"' does not exist"))

[nip5]

• The unit test shouldn't be testing if the directory contains the files before the function is called because that is done in extract_soil_ISRIC250m so it is done regardless of how the function is being called. Basically if I were to do that I'd be checking it twice.

• Having the function fail because the person doesn't have the correct dataset is by design.

[dschlaep]

Having the function fail because a user doesn't have the files is correct; but having the unit tests fail because a developer doesn't have the files is bad -- the unit tests should skip.

[dschlaep]

I'm repeating myself skip doesn't work outside test_that blocks.

Why don't you run the package tests on another machine that doesn't have GriddedGlobalV5 installed? Something like

Sys.setenv(NOT_CRAN = "true")
Sys.setenv(RSFSW2_ALLTESTS = "true")
devtools::test()

[nip5]

It seems to work, do you mean you don't like it outside of testthat?

[dschlaep]

You are correct skip does work correctly outside of test_that blocks -- this must be a new feature of more recent testthat versions because it didn't work in earlier versions when I checked it out originally several years back (e.g., discussion on this r-lib/testthat@b1e41a0).

Apparently, the devel version of testthat also introduced an skip_if_offline (r-lib/testthat@be8e6b6) which will be nice for us!

Ok, I am fine with your skips, but there is still the problem that the tests are not skipped and fail if the correct external files are not available.