Replacing 'testthat' functions #2987

[Its-Maniaco]

This PR replaces the testthat function in mainstream files. Addresses #2987

[Its-Maniaco]

@Aariq @moki1202 @infotroph I tried replacing testthat function with appropriate replacement. Once I get a 👍 from you guys I will proceed with other functions as well.

[moki1202]

@Its-Maniaco This change looks pretty good to me! The arguments structure for the replaced function looks good too. But let's not count on my statement just yet 😉 . Once you get a green light from the other reviewers please make sure you've added the packageName to the respective description file and pushed the newly generated .Rd (documentation) file.

[Aariq]

This is fine as a drop-in replacement—you've got the pattern right. I don't understand what is going on in this particular instance though. The point of assert_that() (or expect_true()) is to throw an error when a condition isn't met. Wrapping it in try() makes it not throw an error. So in this particular instance, I think something like this would make more sense:

eq_units <- PEcAn.utils::units_are_equivalent(ncvar_unit, var_correct_unit)
if(!eq_units) {
  glue::glue("NetCDF unit '{ncvar_unit}' not equivalent to expected unit '{var_correct_unit}'.")
}

[Aariq]

Actually, it should probably use PEcAn.logger() to print the message.

eq_units <- PEcAn.utils::units_are_equivalent(ncvar_unit, var_correct_unit)
if(!eq_units) {
  PEcAn.logger::logger.error(
    glue::glue("NetCDF unit '{ncvar_unit}' not equivalent to expected unit '{var_correct_unit}'.")
  )
}

Errors from PEcAn.logger aren't R errors (I know, confusing) and don't exit out of the function. They just print the message with a timestamp and the word "ERROR"

[moki1202]

@Its-Maniaco any follow up to this?

[Its-Maniaco]

@Aariq @moki1202 try2 <- purrr::partial(try, silent = TRUE) test_dims <- list( try2(testthat::expect_type(dimensions, "list")), can this part be replace with something similar to if(!is.list(dimensions)){stop("dimension is not of type list")}

[Aariq]

@Aariq @moki1202 try2 <- purrr::partial(try, silent = TRUE) test_dims <- list( try2(testthat::expect_type(dimensions, "list")), can this part be replace with something similar to if(!is.list(dimensions)){stop("dimension is not of type list")}

This is from check_met_input_file(), yeah? I think what the current code is trying to do is not to fail fast (which is what your suggestion would do), but rather to collect all the validation errors into one place as text (test_dims_summary$dim_errors) and return them as part of the results object. This is where something from assertthat would likely be a better "drop-in" replacement—maybe validate_that()?

[Aariq]

Remember to also move testthat from "Imports" to "Suggests" in the relevant DESCRIPTION files

[Its-Maniaco]

@Aariq try2(testthat::expect_match( ncdf4::ncatt_get(nc, "time", "units")[["value"]],time_regex)) time_regex I could not figure out a replacement for this.

[Its-Maniaco]

@Aariq @moki1202 @infotroph Is there anything else needed before this PR gets merged?

[infotroph]

Looks like you'll need to run ./scripts/generate_dependencies.R and commit the changes it creates

[mdietze]

Check is failing because of a need to run make document. Tests is failing on load.cfmet.R -- @Its-Maniaco I think you'll want to take a closer look at those issues

[mdietze]

@Its-Maniaco pinging you to hopefully finish up this PR

[mdietze]

@Its-Maniaco wanted to check in about your plans / ability to finish up this PR?

[mdietze]

@Its-Maniaco currently getting a test failure in load.cfmet.R