Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement shinytest2 for tmg #712

Closed
16 of 19 tasks
donyunardi opened this issue Mar 22, 2024 · 6 comments
Closed
16 of 19 tasks

Implement shinytest2 for tmg #712

donyunardi opened this issue Mar 22, 2024 · 6 comments
Assignees
Labels

Comments

@donyunardi
Copy link
Contributor

donyunardi commented Mar 22, 2024

This is a continuation of https://github.com/insightsengineering/coredev-tasks/issues/503

Using the shinytest2 helper class that we built for teal, let extend the feature for tmg.

We have 15 teal module functions in tmg

Note

  • We stick to one app per test and open issue to optimize this by exploring one app for all tests in a module.
  • Create function: app_driver_\<name of module\>
    • create this in tests/testthat/helper-TealAppDriver.R so we can call it as a function in the test case.

Test specs

  • Test if the example apps for the modules tm_* can be run without shiny errors
    • We won't be doing a snapshot test or confirm if the exact output based on the encoding. We will only check if the visualization is generated when the app initialized.
    • Run the app using different arguments other than what's provided in the example (when applicable)
  • Test if visualization is updated when encoding is updated
  • Prefix all the external function (i.e. within and specifying the module, everywhere!)

@insightsengineering/nest-core-dev
As discussed, please self-assign by adding your name to the modules.
Please begin with one or two modules and review them before applying the test cases to the remaining modules.

@averissimo
Copy link
Contributor

Created an initial PR that will support {tmg} testing (#714).

I'm wondering about what datasets should be used as the base for automated testing, (1) iris/mtcars/ or (2) CDISC

CDISC is closer to the overall goal of teal, on the other hand, all the examples modules should work with general data, just as it was recently added to all the examples (a mix between iris/mtcars/CO2/USArrests/...)

Any thoughts folks?

@kartikeyakirar
Copy link
Contributor

I believe general datasets can encompass all necessary functionalities since (tmg) is designed as standard modules for data operations.

I prefer using general datasets to maintain simplicity, except in cases where specific functionalities are only relevant for CDISC datasets

@averissimo
Copy link
Contributor

  • Test if the example apps for the modules tm_* can be run without shiny errors

I think we can focus our effort partially on merging insightsengineering/teal.modules.clinical#983 and then copy/paste it to {tmg}

This will allow our automated tests to be focused on the individual module's functionality (and leave basic error detection to this effort that is 2-in-1

averissimo added a commit that referenced this issue Apr 10, 2024
# Pull Request

Part of #712

#### Changes description

- Allow `TealAppDriver` to be used in tests
  - R6 class is not yet exported from `{teal}` and needs to be used here
- Adds CDISC simple data helper to use in e2e tests
- Adds `skip_if_too_deep` function

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@averissimo
Copy link
Contributor

The automated tests being developed for {tmc} works in this package as well with 2 exceptions [ FAIL 2 | WARN 0 | SKIP 0 | PASS 68 ]

Require custom input at startup (selections are not available on the module's parameters as of now)

  • tm_file_viewer (file being chosen)
  • tm_g_distribution: choosing a test on a collapsed section of encoding

It lives on a tentative branch as of now (477_shinytest2_examples@main)

We could address this in different ways:

  • Exclude example app and create custom test
  • Set missing options via module call (adding new argument)
  • Allow for some shiny validation errors based on a regular expression

@pawelru
Copy link
Contributor

pawelru commented Apr 10, 2024

Thanks for running this in here. Out of the options you listed I think I like allow-list (for warnings / validation / etc.) the most and disliked adding new argument the most :) It would be great to have such list very specific and accurate, e.g. for file XYZ accept validation with ID ABC and warning DEF.

averissimo added a commit that referenced this issue Apr 17, 2024
# Pull Request

<!--- Replace `#nnn` with your issue link for reference. -->

Part of #712 

### Changes description

- Changes `shinyApp` function used in `TealAppDriver` to trick
`shinytest2` into loading the package
- This allows the module server function to call package in `Depends`
without prefixing

#### How to test?

- Checkout branch
- Save test below in the `tests/testthat` directory
- Restart R session (don't load package afterwards!)
- Run `devtools::test(filter = "test-remove_me.R")` or whatever name you
use
- Run test interactively after loading package

```r
# Save in tests/testthat/test-remove_me.R
testthat::test_that("sample test", {

  # general data example
  data <- teal.data::teal_data()
  data <- within(data, {
    require(nestcolor)
    CO2 <- CO2
  })
  teal.data::datanames(data) <- "CO2"

  mod <- teal.modules.general::tm_g_scatterplot(
    label = "Scatterplot Choices",
    x = teal.transform::data_extract_spec(
      dataname = "CO2",
      select = teal.transform::select_spec(
        label = "Select variable:",
        choices = teal.transform::variable_choices(data[["CO2"]], c("conc", "uptake")),
        selected = "conc",
        multiple = FALSE,
        fixed = FALSE
      )
    ),
    y = teal.transform::data_extract_spec(
      dataname = "CO2",
      select = teal.transform::select_spec(
        label = "Select variable:",
        choices = teal.transform::variable_choices(data[["CO2"]], c("conc", "uptake")),
        selected = "uptake",
        multiple = FALSE,
        fixed = FALSE
      )
    ),
    color_by = teal.transform::data_extract_spec(
      dataname = "CO2",
      select = teal.transform::select_spec(
        label = "Select variable:",
        choices = teal.transform::variable_choices(
          data[["CO2"]],
          c("Plant", "Type", "Treatment", "conc", "uptake")
        ),
        selected = NULL,
        multiple = FALSE,
        fixed = FALSE
      )
    ),
    size_by = teal.transform::data_extract_spec(
      dataname = "CO2",
      select = teal.transform::select_spec(
        label = "Select variable:",
        choices = teal.transform::variable_choices(data[["CO2"]], c("conc", "uptake")),
        selected = "uptake",
        multiple = FALSE,
        fixed = FALSE
      )
    ),
    row_facet = teal.transform::data_extract_spec(
      dataname = "CO2",
      select = teal.transform::select_spec(
        label = "Select variable:",
        choices = teal.transform::variable_choices(data[["CO2"]], c("Plant", "Type", "Treatment")),
        selected = NULL,
        multiple = FALSE,
        fixed = FALSE
      )
    ),
    col_facet = teal.transform::data_extract_spec(
      dataname = "CO2",
      select = teal.transform::select_spec(
        label = "Select variable:",
        choices = teal.transform::variable_choices(data[["CO2"]], c("Plant", "Type", "Treatment")),
        selected = NULL,
        multiple = FALSE,
        fixed = FALSE
      )
    ),
    ggplot2_args = teal.widgets::ggplot2_args(
      labs = list(subtitle = "Plot generated by Scatterplot Module")
    )
  )

  app_driver <- init_teal_app_driver(
    data = data,
    modules = list(mod)
  )

  app_driver$expect_no_validation_error()
})
```

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
averissimo added a commit that referenced this issue Apr 18, 2024
# Pull Request

<!--- Replace `#nnn` with your issue link for reference. -->

Part of  #712 

#### Changes description

- prefixes `ggplot2` functions calls
- "content is displayed correctly" test
  - Ensures that all datasets are present in left table
- Ensures that table below the plot has correct first column title
("Statistic" for numeric variables and "levels" for categorical)
- "main output interactivity doesn't show errors" test
  - Tests all buttons and input sliders

#### Notes:

- Click function on javascript elements with callbacks doesn't trigger
them
  - Clicking on variable table row does not select them

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
m7pr added a commit that referenced this issue Apr 18, 2024
Part of #712

---------

Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
m7pr added a commit that referenced this issue Apr 18, 2024
Part of #712

---------

Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
kartikeyakirar added a commit that referenced this issue Apr 19, 2024
Part of
#712

---------

Signed-off-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
kartikeyakirar added a commit that referenced this issue Apr 19, 2024
Part of #712

- [x] tested header-text
- [x] tested table displayed correctly
- [x] additional argument tags.
- [x] checked metadata data

---------

Signed-off-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Vedha Viyash <49812166+vedhav@users.noreply.github.com>
kartikeyakirar added a commit that referenced this issue Apr 19, 2024
Part of
#712

---------

Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
m7pr added a commit that referenced this issue Apr 19, 2024
Part of #712

---------

Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
averissimo added a commit that referenced this issue Apr 19, 2024
Part of #712

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
kartikeyakirar added a commit that referenced this issue Apr 19, 2024
Part of
#712

~~- [ ]  testing table caption (DT functionality)~~
- [x] testing table
- [x] variable selected

---------

Signed-off-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
Signed-off-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: m7pr <marcin.kosinski.mk1@roche.com>
averissimo added a commit that referenced this issue Apr 19, 2024
Part of #712

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: m7pr <marcin.kosinski.mk1@roche.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
averissimo added a commit that referenced this issue Apr 19, 2024
Part of #712

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
kartikeyakirar added a commit that referenced this issue Apr 19, 2024
Part of
#712

---------

Signed-off-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: m7pr <marcin.kosinski.mk1@roche.com>
kartikeyakirar added a commit that referenced this issue Apr 19, 2024
Part of
#712

---------

Signed-off-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
averissimo added a commit that referenced this issue Apr 22, 2024
# Pull Request

Part of #712

### Changes description

- Adds 2 new e2e tests using `shinytest2`
  - Data extract inputs
    - [x] Main
    - [x] Available on specific plots
  - Encoding options via "(guided) monkey typing"
- Fixes typo on module

### Considerations

- End-2-End tests are complex and require complex set of expectations
- Otherwise, we risk having a very long testing pipeline (`AppDriver`
has a relevant start-up time)
- How complete do we want to be on the encoding testing?
- Took a brute force approach here, but small changes/removals will
break the tests

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
Co-authored-by: Dawid Kałędkowski <6959016+gogonzo@users.noreply.github.com>
vedhav added a commit that referenced this issue Apr 22, 2024
Part of #712

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: Marcin <133694481+m7pr@users.noreply.github.com>
averissimo added a commit that referenced this issue Apr 22, 2024
# Pull Request

<!--- Replace `#nnn` with your issue link for reference. -->

Part of #712 

#### Changes description

- Adds tests that iterate on each documentation file and runs the
examples apps by mocking `interactive` and
`shinyApp` functions.
- Checks if there are no errors nor validation errors (with exceptions)
- Implements
insightsengineering/teal.modules.clinical#983 on
this repository

#### Changes from
insightsengineering/teal.modules.clinical#983

- Adds:
  - Regex rules define "accepted" validation errors
- Fixes:
- Reverts to use `library` instead of `pkgload::load_all` due to
problems with `system.file` call that cannot find package files.

```diff
diff -u teal.modules.clinical/tests/testthat/test-examples.R teal.modules.general/tests/testthat/test-examples.R 
--- teal.modules.clinical/tests/testthat/test-examples.R	2024-04-12 10:32:33.100707738 +0200
+++ teal.modules.general/tests/testthat/test-examples.R	2024-04-12 10:26:27.645642183 +0200
@@ -38,12 +38,7 @@ with_mocked_app_bindings <- function(code) {
   # change to `print(shiny__shinyApp(...))` and remove allow warning once fixed
   mocked_shinyApp <- function(ui, server, ...) { # nolint object_name_linter.
     functionBody(server) <- bquote({
-      pkgload::load_all(
-        .(normalizePath(file.path(testthat::test_path(), "..", ".."))),
-        export_all = FALSE,
-        attach_testthat = FALSE,
-        warn_conflicts = FALSE
-      )
+      library(.(testthat::testing_package()), character.only = TRUE)
       .(functionBody(server))
     })
     print(do.call(shiny__shinyApp, append(x = list(ui = ui, server = server), list(...))))
@@ -56,16 +51,34 @@ with_mocked_app_bindings <- function(code) {
     app_driver <- shinytest2::AppDriver$new(
       x,
       shiny_args = args,
+      timeout = 20 * 1000,
+      load_timeout = 30 * 1000,
       check_names = FALSE, # explicit check below
       options = options() # rstudio/shinytest2#377
     )
     on.exit(app_driver$stop(), add = TRUE)
-    app_driver$wait_for_idle(timeout = 20000)
+    app_driver$wait_for_idle()
 
     # Simple testing
     ## warning in the app does not invoke a warning in the test
     ## rstudio/shinytest2#378
     app_logs <- subset(app_driver$get_logs(), location == "shiny")[["message"]]
+
+    # Check if the teal app has content (indicator of a Shiny App fatal error)
+    if (identical(trimws(app_driver$get_text("#teal-main_ui_container")), "")) {
+      tryCatch(
+        app_driver$wait_for_idle(duration = 2000), # wait 2 seconds for session to disconnect
+        error = function(err) {
+          stop(
+            sprintf(
+              "Teal Application is empty. An Error may have occured:\n%s",
+              paste0(subset(app_driver$get_logs(), location == "shiny")[["message"]], collapse = "\n")
+            )
+          )
+        }
+      )
+    }
+
     # allow `Warning in file(con, "r")` warning coming from pkgload::load_all()
     if (any(grepl("Warning in.*", app_logs) & !grepl("Warning in file\\(con, \"r\"\\)", app_logs))) {
       warning(
@@ -79,9 +92,17 @@ with_mocked_app_bindings <- function(code) {
     ## Throw an error instead of a warning (default `AppDriver$new(..., check_names = TRUE)` throws a warning)
     app_driver$expect_unique_names()
 
+    err_el <- Filter(
+      function(x) {
+        allowed_errors <- getOption("test_examples.discard_error_regex", "")
+        identical(allowed_errors, "") || !grepl(allowed_errors, x)
+      },
+      app_driver$get_html(".shiny-output-error")
+    )
+
     ## shinytest2 captures app crash but teal continues on error inside the module
     ## we need to use a different way to check if there are errors
-    if (!is.null(err_el <- app_driver$get_html(".shiny-output-error"))) {
+    if (!is.null(err_el) && length(err_el) > 0) {
       stop(sprintf("Module error is observed:\n%s", err_el))
     }
 
@@ -110,11 +131,14 @@ with_mocked_app_bindings <- function(code) {
 
 strict_exceptions <- c(
   # r-lib/gtable#94
-  "tm_g_barchart_simple.Rd",
-  "tm_g_ci.Rd",
-  "tm_g_ipp.Rd",
-  "tm_g_pp_adverse_events.Rd",
-  "tm_g_pp_vitals.Rd"
+  "tm_outliers.Rd",
+  "tm_g_response.Rd",
+  "tm_a_pca.Rd"
+)
+
+discard_validation_regex <- list(
+  "tm_file_viewer.Rd" = "Please select a file\\.",
+  "tm_g_distribution.Rd" = "Please select a test"
 )
 
 for (i in rd_files()) {
@@ -122,11 +146,18 @@ for (i in rd_files()) {
     paste0("example-", basename(i)),
     {
       testthat::skip_on_cran()
+      skip_if_too_deep(5)
       if (basename(i) %in% strict_exceptions) {
         op <- options()
         withr::local_options(opts_partial_match_old)
         withr::defer(options(op))
       }
+      # Allow for specific validation errors for individual examples
+      withr::local_options(
+        list(
+          "test_examples.discard_error_regex" = discard_validation_regex[[basename(i)]]
+        )
+      )
       with_mocked_app_bindings(
         # suppress warnings coming from saving qenv insightsengineering/teal.code#194
         suppress_warnings(```

---------

Signed-off-by: André Veríssimo <211358+averissimo@users.noreply.github.com>
Co-authored-by: kartikeya kirar <kirar.kartikeya1@gmail.com>
Co-authored-by: Vedha Viyash <49812166+vedhav@users.noreply.github.com>
@averissimo
Copy link
Contributor

All PRs have been merged. Great job team!! 🎉 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants