Refactoring unit tests

[mattroumaya]

Originally, I thought it would be a good idea to write a few helper functions to create unit tests for similar functions (e.g., *_Map_Raw() or *_Assess(). After doing some more research, it seems like we do have a good initial framework for unit tests, where each testthat file contains tests that are specific to a function.

---

tl;dr

my suggestion is to:

1. Refactor unit tests so they are standardized and easier to read
2. For unit tests that expect an error, use expect_snapshot_* until the expected error is solidified
3. Create documentation about standard unit tests for a given type of function
4. Spend focused time developing nuanced unit tests once we get closer to a release, perhaps as part of a pre-QC process.

---

Here are some ideas to plan for the future state of {gsm}, improve current unit tests, and document some process improvements and/or general guidelines:

Use expect_snapshot until function is in steady-state:

• expect_snapshot and associated "snapshot" tests can be useful for capturing an error message or other hard-to-document output, and saving it in a separate .md file found in tests\testthat\_snaps
• Once a function makes it into a main release, or maybe even a major release (or whenever we determine), we could consider part of qualification or due-diligence that we refactor unit tests to expect specific warning or error messages. This would be somewhat tedious, but would only need to be done for high-risk and/or high-impact functions.

Example:

Instead of including strings of expected error messages like this:

expect_error(
  AE_Map_Raw(
    list(), list()) 
  %>% suppressMessages, 
  "Errors found in dfAE."
  )

We can instead use:

expect_snapshot_error(
  AE_Map_Raw(
    list(), list()
  )
)

Which will capture the most recent message and document in a separate .md file as:

# incorrect inputs throw errors

    Errors found in dfAE.

Document standard assumptions for a given type of function, and ensure unit tests exist for each function

• For instance, all *_Map functions must contain tests for:
  • output created as expected and has correct structure
  • incorrect inputs throw errors
  • error given if required column not found

The suggestion here is to add section to the Wiki page or document elsewhere a unit testing standard for each type of function.

• The standard would be the minimum, and specific tests would definitely need to be developed for utility functions and other specialty functions.

Table below shows tests where Count > 1 where we do have some consistency, but will likely benefit from documenting a standard:

Test	Count
incorrect inputs throw errors	21
output created as expected and has correct structure	12
error given if required column not found	9
correct function and params are returned	4
invalid lTags throw errors	4
problematic lTags names are caught	4
NA values are caught	3
output is created as expected	3
AE assessment can return a correctly assessed data frame for the wilcoxon test grouped by the study variable when given correct input data from clindata and the results should be flagged correctly using a custom threshold.	2
all data is mapped and summarized correctly	2
Consent assessment can return a correctly assessed data frame grouped by the study variable when given correct input data from clindata and the results should be flagged correctly	2
custom mapping runs without errors	2
NAs are handled correctly	2
output is correct given clindata example input	2
output is correct given example input	2
Test that (NA, NaN) in input exposure data throws a warning and drops the participant(s) from the analysis.	2

Resources:

• Example of snapshot testing in dplyr
• Creating your own testing tools
• a nice blog post
• curated list of potentially helpful tools, to use or get ideas from
• A more theoretical paper about unit testing from Hadley Wickham

To create table above:

library(gt)
library(tidyverse)

standardTests <- function() {
testResults <- testthat::test_local()
func <- map2(testResults, "context", pluck)
description <- map2(testResults, "test", pluck)

tests <- bind_cols(
  Reduce(rbind.data.frame, func),
  Reduce(rbind.data.frame, description)
) %>% 
  rename(Function = 1,
         Test = 2) %>% 
  arrange(Function, Test)

return(tests)

}


tests <- standardTests()

idx <- which(tests$Test %in% c("all data is mapped and summarized correctly",
                    "custom mapping runs without errors",
                    "error given if required column not found",
                    "incorrect inputs throw errors",
                    "NA values are caught",
                    "output created as expected and has correct structure",
                    "output is created as expected"), 
             arr.ind = T)

# subset where count > 1
tests %>% 
  group_by(Test) %>% 
  summarize(Count = n()) %>% 
  arrange(desc(Count)) %>% 
  filter(Count > 1) %>% 
  gt::gt() %>% 
  gt::as_raw_html()

# all tests and functions
tests %>% 
  gt::gt() %>% 
  gt::tab_style(
    style = list(
      gt::cell_fill(color = "#C5B4E3")
    ),
    locations = cells_body(
      columns = Test,
      rows = idx)
  ) %>% 
  gt::gt()

cc: @jwildfire @kodesiba @gwu05

[jwildfire]

Document standard assumptions for a given type of function, and ensure unit tests exist for each function

• For instance, all *_Map functions must contain tests for:

  • output created as expected and has correct structure
  • incorrect inputs throw errors
  • error given if required column not found

I agree this is a good place to focus.

The suggestion here is to add section to the Wiki page or document elsewhere a unit testing standard for each type of function.

Documenting these expectations is a good place to start, but I was thinking of taking it one step further and creating a function or functions that check for these common requirements in every assessment. I guess we could use the framework in this link to write expect_assessement_structure etc. and then call them first in every assessment.

The more I think about it, the more I'm on the fence about it this "functionalization" step may or may not be worth the effort for v1.0.

[jwildfire]

The other main thing on my mind is leaning more heavily on the testing of is_mapping_valid() and removing overlapping tests from the map_raw and assessment unit tests. I think our general rule should be that if something is tested in the unit tests of is_mapping_valid() it doesn't need to be repeatedly tested in downstream functions.

[jwildfire]

For an actionable next step, I suggest a quick "pilot" PR to refactor the test cases for AE_map_raw based on the discussion above.

[jwildfire]

On a more general note, I think our testing framework is already way past the threshold needed for v1 QC, so my goals for this refactor are more about process than quality. here is to make the existing tests easier to maintain and update, and to set up a process framework that makes it relatively straightforward to spin up new tests.