Package review - TESTING

Review Comments

On `devtools::check()`, I see:

❯ checking R code for possible problems ... NOTE
  check_plausibility_mfaz: warning in sexRatioTest({: partial argument
    match of 'code' to 'codes'
  check_plausibility_mfaz: warning in {: partial argument match of 'code'
    to 'codes'
  check_plausibility_mfaz: warning in sex: partial argument match of
    'code' to 'codes'
  check_plausibility_mfaz: warning in }: partial argument match of 'code'
    to 'codes'
  check_plausibility_mfaz: warning in }, code = c(1, 2)): partial
    argument match of 'code' to 'codes'
  check_plausibility_muac: warning in sexRatioTest({: partial argument
    match of 'code' to 'codes'
  check_plausibility_muac: warning in {: partial argument match of 'code'
    to 'codes'
  check_plausibility_muac: warning in sex: partial argument match of
    'code' to 'codes'
  check_plausibility_muac: warning in }: partial argument match of 'code'
    to 'codes'
  check_plausibility_muac: warning in }, code = c(1, 2)): partial
    argument match of 'code' to 'codes'
  check_plausibility_wfhz: warning in sexRatioTest({: partial argument
    match of 'code' to 'codes'
  check_plausibility_wfhz: warning in {: partial argument match of 'code'
    to 'codes'
  check_plausibility_wfhz: warning in sex: partial argument match of
    'code' to 'codes'
  check_plausibility_wfhz: warning in }: partial argument match of 'code'
    to 'codes'
  check_plausibility_wfhz: warning in }, code = c(1, 2)): partial
    argument match of 'code' to 'codes'

0 errors ✔ | 0 warnings ✔ | 1 note ✖

To learn about partial matching in R, see https://stat.ethz.ch/pipermail/r-devel/2024-April/083344.html.

For this I would recommend using the correct name of the argument code to codes.

[ ] #22
[ ] #23

on `devtools::check_win_devel()`, results have been emailed to `tomas.zaba@outlook.com`

on `devtools::check_win_oldrelease()`, results have been emailed to `tomas.zaba@outlook.com`

on `devtools::check_win_release()`, results have been emailed to `tomas.zaba@outlook.com`

[ ] #24

on `rhub::rhub_check()`

[ ] #25

on `devtools::test_coverage()`, I see:

File    Lines   Relevant    Covered Missed  Hits / Line Coverage
R/prevalence_wfhz.R 339 110 91  19  4   82.73%
R/prevalence_muac.R 376 114 100 14  4   87.72%
R/prevalence_mfaz.R 202 60  53  7   2   88.33%
R/prevalence_combined.R 244 84  80  4   5   95.24%
R/data_processors.R 337 64  62  2   13  96.88%
R/case_definitions.R    270 87  87  0   10  100.00%
R/quality_checkers.R    224 67  67  0   2   100.00%
R/quality_scorers.R 219 58  58  0   8   100.00%
R/pretty_tables.R   180 53  53  0   1   100.00%
R/quality_classifiers.R 246 52  52  0   17  100.00%
R/age.R 152 29  29  0   13  100.00%
R/sample_size.R 72  11  11  0   1   100.00%

[ ] TESTING: review code coverage and see if uncovered lines can be tested

on review of your testing suite

You are fond of using local() - not sure where you got this - I reviewd Hadley's work again and this is not something from him. Whilst I can see the reason for using local(), it needs to be made clear as well that the testthat() mechanism runs in its own environment separate from the package environment (see https://r-pkgs.org/testing-design.html#sec-testing-design-principles)
I suggest moving the top level code you have been putting inside local() into the test context itself like this:

testthat::test_that("compute_month_to_days() does the job as expected", {
  ## Sample data ----
  age_mo <- seq(6,23)
  df <- data.frame(age_mo)

  ## Expected results ----
  df[["expected_results"]] <- c(
    182.6250, 213.0625, 243.5000, 273.9375, 304.3750, 334.8125, 365.2500,
    395.6875, 426.1250, 456.5625, 487.0000, 517.4375, 547.8750, 578.3125,
    608.7500, 639.1875, 669.6250, 700.0625
  )

  ## Observed results ----
  age_days <- compute_month_to_days(age_mo)
  df[["age_days"]] <- age_days

  testthat::expect_vector(df[["age_days"]], size = 18)
  testthat::expect_equal(df[["age_days"]], df[["expected_results"]])
})

[ ] #26

On testing whether inputs to functions are of the right class/type

in the package-review branch, I created a test called test-input-checks.R to show a minimal example of class/type checking for functions.

test_that("inputs are checked for type and/or class", {
  ## Age functions (exported only) ----
  test_data <- data.frame(
    survey_date = Sys.Date(),
    dob = c(
      "2000-01-01", "2020-01-01", "01-01-2022", "Jan-01-2021",
      "01-January-2023", "January-01-2021", "20-01-01", "01-01-22"
    ),
    age = NA_character_
  )

  expect_error(
    process_age(
      df = test_data, svdate = "survey_date", birdate = "dob", age = "age"
    ),
    regexp = "dob not a Date object"
  )
})

With this test, what I want to reinforce is this idea that functions needs to check for the input class/type to ensure that what is being provided by user is correct input.

In your process_age() function, you have 2 parameters that require dates but in your documentation, you don't indicate that these dates need to be Date objects. You even say in your documenation "day, month year" so users might think this is the format that dates need to be given. The test I created above tests whether your function will throw an appropriate error if the dates in the input are in different formats and are character class/type.

Your package doesn't throw its own error. It errors on age because age in NAcharacter which again your package should specific error on with a message rather than relying on an obscure base error message.

[ ] #27
[ ] #28

nutriverse / mwana