search

pharmaverse / admiral

ADaM in R Asset Library
https://pharmaverse.github.io/admiral
Apache License 2.0
215 stars 60 forks source link

Bug: testthat warnings on main branch #2472

Closed esimms999 closed 2 weeks ago

esimms999 commented 3 weeks ago

What happened?

Testthat warning being thrown for:

Session Information

==> devtools::test()

ℹ Testing admiral ✔ | F W S OK | Context ✔ | 6 | admiral_options
✔ | 8 | call_derivation [1.3s]
✔ | 1 | call_user_fun
✔ | 4 | compute_age_years
✔ | 20 | compute_duration
✔ | 8 | compute_framingham
✔ | 7 | compute_kidney
✔ | 26 | compute_qual_imputation
✔ | 7 | compute_scale
✔ | 4 | consolidate_metadata
✔ | 1 | create_country_codes
✔ | 23 | create_query_data [1.8s]
✔ | 12 | create_single_dose_dataset [2.4s]
✔ | 58 | derive_advs_params [3.3s]
✔ | 2 | derive_basetype_records
✔ | 4 | derive_expected_records
✔ | 12 | derive_extreme_event [3.6s]
✔ | 9 | derive_extreme_records [1.0s]
✔ | 13 | derive_joined [2.9s]
✔ | 6 | derive_locf_records
✔ | 2 33 | derive_merged [3.1s]
─────────────────────────────────────────────────────────────────────────────────────────────────────── Warning (test-derive_merged.R:692:3): derive_var_merged_summary Test 28: error when relatioship is incorrectly specificed 'one-to-one' Adding new snapshot: Code derive_vars_merged(advs, dataset_add = adsl, by_vars = exprs(USUBJID), new_vars = exprs(SEX), relationship = "one-to-one") Condition Error in tryCatch(): ! Each row in dataset_add must match at most 1 row in dataset. i Row 1 of dataset_add matches multiple rows in dataset.

Warning (test-derive_merged.R:706:3): derive_var_merged_summary Test 29: merge selected variables with relatioship as 'one-to-one' Adding new snapshot: Code derive_vars_merged(adsl, dataset_add = advs, by_vars = exprs(USUBJID), new_vars = exprs(WEIGHTBL = AVAL), filter_add = AVISIT == "BASELINE", relationship = "one-to-one") Output

A tibble: 4 x 5

USUBJID SEX   COUNTRY STUDYID WEIGHTBL
<chr>   <chr> <chr>   <chr>      <dbl>

1 ST42-1 F AUT ST42 66 2 ST42-2 M MWI ST42 88 3 ST42-3 M NOR ST42 NA 4 ST42-4 F UGA ST42 NA ─────────────────────────────────────────────────────────────────────────────────────────────────────── ✔ | 14 | derive_param_computed [1.2s]
✔ | 2 | derive_param_doseint
✔ | 2 | derive_param_exist_flag
✔ | 5 | derive_param_exposure
✔ | 4 | derive_param_extreme_record
✔ | 1 | derive_param_framingham
✔ | 6 | derive_param_qtc
✔ | 1 | derive_param_rr
✔ | 14 | derive_param_tte [4.5s]
✔ | 6 | derive_param_wbc_abs
✔ | 7 | derive_summary_records
✔ | 3 | derive_var_analysis_ratio
✔ | 8 | derive_var_anrind
✔ | 123 | derive_var_atoxgr [8.9s]
✔ | 4 | derive_var_base
✔ | 3 | derive_var_chg
✔ | 11 | derive_var_dthcaus [2.8s]
✔ | 6 | derive_var_extreme_date [1.4s]
✔ | 9 | derive_var_extreme_flag
✔ | 8 | derive_var_joined_exist_flag [2.0s]
✔ | 3 | derive_var_merged_ef_msrc
✔ | 5 | derive_var_obs_number
✔ | 16 | derive_var_ontrtfl
✔ | 2 | derive_var_relative_flag
✔ | 4 | derive_var_shift
✔ | 1 | derive_var_trtdurd
✔ | 10 | derive_var_trtemfl [1.3s]
✔ | 10 | derive_vars_aage
✔ | 5 | derive_vars_computed
✔ | 12 | derive_vars_dt_dtm_utils
✔ | 26 | derive_vars_dt [1.4s]
✔ | 2 | derive_vars_dtm_to_dt
✔ | 2 | derive_vars_dtm_to_tm
✔ | 34 | derive_vars_dtm [1.8s]
✔ | 5 | derive_vars_duration
✔ | 10 | derive_vars_dy
✔ | 1 | derive_vars_extreme_event
✔ | 18 | derive_vars_query [2.4s]
✔ | 1 5 | derive_vars_transposed
─────────────────────────────────────────────────────────────────────────────────────────────────────── Warning (test-derive_vars_transposed.R:58:3): derive_vars_transposed Test 3: filtering the merge dataset works with relationship 'many-to-one' Adding new snapshot: Code derive_vars_transposed(dataset, dataset_merge, by_vars = exprs(USUBJID), key_var = TESTCD, value_var = VALUE, filter = TESTCD == "T01", relationship = "many-to-one") Output

A tibble: 3 x 3

USUBJID  VAR1   T01
<chr>   <dbl> <dbl>

1 P01 3 31 2 P02 31 3 3 P03 42 NA ─────────────────────────────────────────────────────────────────────────────────────────────────────── ✔ | 6 | dt_level
✔ | 4 | duplicates
✔ | 2 | event
✔ | 2 | filter_exist
✔ | 2 | filter_extreme
✔ | 8 | filter_joined [1.2s]
✔ | 8 | filter_relative [1.4s]
✔ | 3 | get_flagged_records
✔ | 3 | get_summary_records
✔ | 13 | period_dataset [1.4s]
✔ | 4 | restrict_derivation
✔ | 5 | roxygen2
✔ | 5 | slice_derivation
✔ | 8 | user_helpers
✔ | 20 | user_utils

══ Results ════════════════════════════════════════════════════════════════════════════════════════════ Duration: 66.3 s

[ FAIL 0 | WARN 3 | SKIP 0 | PASS 765 ]

Way to go!

Reproducible Example

No response

bundfussr commented 3 weeks ago

I think we had the issue before that R-CMD passes although the unit tests produce warnings.

The warnings are caused by line breaks in the unit test description. For example

test_that("derive_var_merged_summary Test 28: error when relatioship is
          incorrectly specificed 'one-to-one'", {
  expect_snapshot(
    derive_vars_merged(advs,
      dataset_add = adsl,
      by_vars = exprs(USUBJID),
      new_vars = exprs(SEX),
      relationship = "one-to-one"
    ),
    error = TRUE
  )

If there are line breaks, expect_snapshot() does not work properly. When run, it adds a new snapshot. But in the next run the snapshot is not recognized and and it issues a warning again that a new snapshot is added.

bms63 commented 3 weeks ago

Ah this makes sense to me - I was going crazy with some these snapshot tests right before release. @ddsjoberg any thoughts on how we might remedy. Be nice to not have to use #nolint

ddsjoberg commented 3 weeks ago

I haven't seen line breaks cause issues in unit testing before. I have MANY MANY line breaks in my projects (https://github.com/ddsjoberg/standalone/blob/main/R/standalone-checks.R) and also many snapshot unit tests, and I haven't seen an issue like this.

bundfussr commented 3 weeks ago

I haven't seen line breaks cause issues in unit testing before. I have MANY MANY line breaks in my projects (https://github.com/ddsjoberg/standalone/blob/main/R/standalone-checks.R) and also many snapshot unit tests, and I haven't seen an issue like this.

Line breaks in the code of the unit tests don't cause any issues. Line breaks in the description of the unit test (in the desc argument of test_that()) are causing issues. I observed this several times when I updated admiral due to the changes in the assert_*() functions. To fix it I've shortened the description. For example

test_that("derive_var_merged_summary Test 28: error if relationship don't match data", {
  expect_snapshot(
    derive_vars_merged(advs,
      dataset_add = adsl,
      by_vars = exprs(USUBJID),
      new_vars = exprs(SEX),
      relationship = "one-to-one"
    ),
    error = TRUE
  )
})

should solve the issue.

bms63 commented 3 weeks ago

@pharmaverse/admiral @pharmaverse/admiral_comm a quick fix! anyone interested?

esimms999 commented 2 weeks ago

Why is test-compute_framingham.R not throwing warnings?

image

esimms999 commented 2 weeks ago

This is interesting. The _snaps/derive_merged.md looks like it does not know the second line of the description is part of the description (lack of leading # ). And the test-compute_framingham.R, while it has multi-line desc, does not make use of expect_snapshot() tests ... and, therefore, does not have an entry in _snaps.

image

esimms999-gsk commented 2 weeks ago

Known issue on testthat repo: https://github.com/r-lib/testthat/issues/1900