Bug: testthat warnings on main branch

[esimms999]

What happened?

Testthat warning being thrown for:

• derive_var_merged_summary tests 28 and 29
• derive_vars_transposed test 3

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]

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]

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]

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]

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]

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

[esimms999]

Why is test-compute_framingham.R not throwing warnings?

[esimms999]

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.

[esimms999-gsk]

Known issue on testthat repo: r-lib/testthat#1900