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

Breaking changes to Appdriver tests with new version 0.2.1 #330

Closed
matt-sd-watson opened this issue Mar 1, 2023 · 23 comments · Fixed by #385
Closed

Breaking changes to Appdriver tests with new version 0.2.1 #330

matt-sd-watson opened this issue Mar 1, 2023 · 23 comments · Fixed by #385
Assignees

Comments

@matt-sd-watson
Copy link

With the release of 0.2.1, my R package test (which contains a Shiny app) is breaking. Below is the code for the test:

require(cytosel)

test_that("{shinytest2} recording: cytosel", {
  testthat::local_edition(3)

  # set the app depending on the interactive execution to avoid a no package found error
  if (interactive()) {
    cytosel_app <- test_path("../../")
  } else {
    cytosel_app <- cytosel::cytosel()
  }
  
  announce_snapshot_file("cytosel-001.png")
  announce_snapshot_file("cytosel-002.png")
  app <- AppDriver$new(cytosel_app,
                       variant = platform_variant(), name = "cytosel", height = 732,
      width = 1161, load_timeout = 1e+05
      # shinyOptions = list(test.mode = TRUE)
      )
    
    # Sys.sleep(7)
    app$wait_for_value(output = "cytosel_logo")
    # IMPORTANT: only run the tests non-interactively using Github actions
    # currently devtools test and devtools check give different screenshot results
    skip_if(interactive())
    app$expect_screenshot()

    skip_if(interactive())
    app$click("help_guide")
    Sys.sleep(7)
    app$expect_screenshot()

})

Where `cytosel' is the name of the package and the function that calls the Shiny app.

Previously, this test passed with v0.2.0 in Github Actions, but now the following error occurs:

══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-shinytest2.R:16'): {shinytest2} recording: cytosel ─────────────
Error in `app_initialize(self, private, app_dir = app_dir, ..., load_timeout = load_timeout, 
    timeout = timeout, wait = wait, expect_values_screenshot_args = expect_values_screenshot_args, 
    screenshot_args = screenshot_args, check_names = check_names, 
    name = name, variant = variant, view = view, height = height, 
    width = width, seed = seed, clean_logs = clean_logs, shiny_args = shiny_args, 
    render_args = render_args, options = options)`: Identified global objects via static code inspection (new("standardGeneric", .Data = function (x, decreasing = FALSE,; ...); standardGeneric("sort"), generic = structure("sort", package = "BiocGenerics"),; package = "BiocGenerics", group = list(), valueClass = character(0),; signature = "x", default = new("derivedDefaultMethod", .Data = function (x,; decreasing = FALSE, ...); ...; package = "methods"), defined = new("signature", .Data = "ANY",; names = "x", package = "methods"), generic = structure("sort", package = "base")))(x,; decreasing, ...))). Failed to locate global object in the relevant environments: 'x'


ℹ You can inspect the failed AppDriver object via `rlang::last_error()$app`
ℹ AppDriver logs:
{shinytest2} R info 21:13:19.25 Start AppDriver initialization
{shinytest2} R info 21:13:19.95 Error while initializing AppDriver:
                                Identified global objects via static code inspection (new("standardGeneric", .Data = function (x, decreasing = FALSE,; ...); standardGeneric("sort"), generic = structure("sort", package = "BiocGenerics"),; package = "BiocGenerics", group = list(), valueClass = character(0),; signature = "x", default = new("derivedDefaultMethod", .Data = function (x,; decreasing = FALSE, ...); ...; package = "methods"), defined = new("signature", .Data = "ANY",; names = "x", package = "methods"), generic = structure("sort", package = "base")))(x,; decreasing, ...))). Failed to locate global object in the relevant environments: 'x'


Caused by error in `globalsByName()`:
! Identified global objects via static code inspection (new("standardGeneric", .Data = function (x, decreasing = FALSE,; ...); standardGeneric("sort"), generic = structure("sort", package = "BiocGenerics"),; package = "BiocGenerics", group = list(), valueClass = character(0),; signature = "x", default = new("derivedDefaultMethod", .Data = function (x,; decreasing = FALSE, ...); ...; package = "methods"), defined = new("signature", .Data = "ANY",; names = "x", package = "methods"), generic = structure("sort", package = "base")))(x,; decreasing, ...))). Failed to locate global object in the relevant environments: 'x'
Backtrace:
     ▆
  1. ├─AppDriver$new(...) at test-shinytest2.R:16:2
  2. │ └─shinytest2 (local) initialize(...)
  3. │   └─shinytest2:::app_initialize(...)
  4. │     ├─base::withCallingHandlers(...)
  5. │     └─shinytest2:::app_initialize_(self, private, ..., view = view)
  6. │       └─shinytest2:::app_save(app_dir)
  7. │         └─shinytest2:::app_data(app)
  8. │           └─shinytest2:::app_server_globals(server)
  9. │             └─globals::globalsOf(server, envir = environment(server), recursive = TRUE)
 10. │               └─globals::globalsOf(...)
 11. │                 └─base::tryCatch(...)
 12. │                   └─base (local) tryCatchList(expr, classes, parentenv, handlers)
 13. │                     └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 14. │                       └─value[[3L]](cond)
 15. │                         └─base::stop(ex)
 16. └─shinytest2 (local) `<fn>`(`<smplErrr>`)
 17.   └─shinytest2:::app_abort(...)
 18.     └─rlang::abort(..., app = self, call = call)
@MichalLauer
Copy link

MichalLauer commented Oct 6, 2023

Hello,

did you manage to fix this issue? I'm currently struggling with the same thing.

To maybe add more context to this issue, I am using the {dplyr} package for data transformation. dplyr uses data masking for variable selection, which is an issue for R CMD Check, so I handle this with the use of

utils::globalVariables(c(
  "matches", "rok", "thead", "tr", "th", "red_izo"
))

as suggested by the R Packages (2e) book.
A shortened version of an error that my app throws is

...
Failed to locate global object in the relevant environments: 'rok'
...

but it's basically identical.

@matt-sd-watson
Copy link
Author

Hello @MichalLauer, I did not end up resolving the issue yet, I simply downgraded back to v0.2.0 for my CI/CD tests.

@MichalLauer
Copy link

Thanks for letting me know @matt-sd-watson.
I tried using your approach, but it still keeps crashing with the same message.
Hopefully, this will get fixed further down the line :)

@gadenbuie
Copy link
Member

The biggest hurdle we currently in our path toward fixing this is that of needing a minimal, reproducible example. If anyone facing this issue could help us by providing a self-contained minimal example that reproduces this issue, it would be much appreciated.

@MichalLauer
Copy link

Sorry, sure thing! Here is a reprex. Note that the .Rprofile is detected because I use {renv}.

Local .Rprofile detected at C:\Users\Mike\Documents\Projekty\RWorkingDirectory\.Rprofile

library(shiny)
#> Warning: package 'shiny' was built under R version 4.3.1
library(dplyr)
#> Warning: package 'dplyr' was built under R version 4.3.1
#> 
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#> 
#>     filter, lag
#> The following objects are masked from 'package:base':
#> 
#>     intersect, setdiff, setequal, union

app_run <- function() {
  ui <- fluidPage(
    textOutput("txt")
  )
  server <- function(input, output, session) {
    output$txt <- renderText({
      data <-
        mtcars |>
        filter(cyl == 6)

      paste0("For ", data$cyl, " the mean mpg is ", mean(data$mpg))
    })
  }
  shinyApp(ui, server)
}

library(testthat)
#> Warning: package 'testthat' was built under R version 4.3.1
#> 
#> Attaching package: 'testthat'
#> The following object is masked from 'package:dplyr':
#> 
#>     matches
library(shinytest2)
#> Warning: package 'shinytest2' was built under R version 4.3.1
Sys.setenv("CHROMOTE_CHROME" = r"(C:\Program Files\BraveSoftware\Brave-Browser\Application\brave.exe)")

test_that("Shiny app works", {
  app <- app_run()
  driver <- AppDriver$new(app)
})
#> ── Error: Shiny app works ──────────────────────────────────────────────────────
#> Error in `app_initialize(self, private, app_dir = app_dir, ..., load_timeout = load_timeout, 
#>     timeout = timeout, wait = wait, expect_values_screenshot_args = expect_values_screenshot_args, 
#>     screenshot_args = screenshot_args, check_names = check_names, 
#>     name = name, variant = variant, view = view, height = height, 
#>     width = width, seed = seed, clean_logs = clean_logs, shiny_args = shiny_args, 
#>     render_args = render_args, options = options)`: Identified global objects via static code inspection (function (input, output, session); {; output$txt <- renderText({; data <- filter(mtcars, cyl == 6); paste0("For ", data$cyl, " the mean mpg is ", mean(data$mpg)); }); }). Failed to locate global object in the relevant environments: 'cyl'
#> 
#> 
#> ℹ You can inspect the failed AppDriver object via `rlang::last_error()$app`
#> ℹ AppDriver logs:
#> {shinytest2} R info 10:23:01.81 Start AppDriver initialization
#> {shinytest2} R info 10:23:01.87 Error while initializing AppDriver:
#>                                 Identified global objects via static code inspection (function (input, output, session); {; output$txt <- renderText({; data <- filter(mtcars, cyl == 6); paste0("For ", data$cyl, " the mean mpg is ", mean(data$mpg)); }); }). Failed to locate global object in the relevant environments: 'cyl'
#> 
#> 
#> Caused by error in `globalsByName()`:
#> ! Identified global objects via static code inspection (function (input, output, session); {; output$txt <- renderText({; data <- filter(mtcars, cyl == 6); paste0("For ", data$cyl, " the mean mpg is ", mean(data$mpg)); }); }). Failed to locate global object in the relevant environments: 'cyl'
#> Backtrace:
#>      ▆
#>   1. ├─AppDriver$new(app)
#>   2. │ └─shinytest2 (local) initialize(...)
#>   3. │   └─shinytest2:::app_initialize(...)
#>   4. │     ├─base::withCallingHandlers(...)
#>   5. │     └─shinytest2:::app_initialize_(self, private, ..., view = view)
#>   6. │       └─shinytest2:::app_save(app_dir)
#>   7. │         └─shinytest2:::app_data(app)
#>   8. │           └─shinytest2:::app_server_globals(server)
#>   9. │             └─globals::globalsOf(server, envir = environment(server), recursive = TRUE)
#>  10. │               └─base::tryCatch(...)
#>  11. │                 └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>  12. │                   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>  13. │                     └─value[[3L]](cond)
#>  14. │                       └─base::stop(ex)
#>  15. └─shinytest2 (local) `<fn>`(`<smplErrr>`)
#>  16.   └─shinytest2:::app_abort(...)
#>  17.     └─rlang::abort(..., app = self, call = call)
#> Error:
#> ! Test failed
#> Backtrace:
#>      ▆
#>   1. └─testthat::test_that(...)
#>   2.   └─withr (local) `<fn>`(`<env>`)
#>   3.     ├─base::tryCatch(...)
#>   4.     │ └─base (local) tryCatchList(expr, classes, parentenv, handlers)
#>   5.     │   └─base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
#>   6.     │     └─base (local) doTryCatch(return(expr), name, parentenv, handler)
#>   7.     └─base::eval(handler$expr, handler$envir)
#>   8.       └─base::eval(handler$expr, handler$envir)
#>   9.         └─reporter$stop_if_needed()
#>  10.           └─rlang::abort("Test failed", call = NULL)

Created on 2023-10-07 with reprex v2.0.2

@gadenbuie
Copy link
Member

Thanks for the reprex! The source of the problem is that shinytest2 will search your code to find any global variables that might be needed to run your code. But because dplyr uses meta-programming, our static analysis of the app code sees cyl and thinks that it's a global variable.

There are two quick ways that you can deal with this problem:

  1. Reference .data$cyl instead of cyl: filter(.data$cyl == 6). This uses the .data pronoun to make it explicit that cyl is from the data frame passed to filter(). Read more about the .data pronoun in the {rlang} docs.

  2. You can "trick" the static analysis by adding cyl <- NULL, either at the top of app_run() or inside the renderText(). I'd recommend this method only if you have a large codebase and don't want to go around adding .data$ to each column name reference.


The above is a way out of the current problem, but we may be able to solve this inside shinytest2. @schloerke, this error comes from globals::globalsOf(), which has a mustExist argument. Could we set this to FALSE? From the docs, it appears that this would exclude unlocatable global variables from the globals list, which sounds okay to me and might be better than the current error.

@MichalLauer
Copy link

Thanks for the update. I can confirm that both solutions work for me. Looking forward to a potential fix:)

What I find interesting is that even tho cyl <- NULL and .data$cyl works, defining cyl as a global variable does not.

app_run <- function() {
  utils::globalVariables("cyl") # <- Defining as a global variable
  ui <- fluidPage(...) # same content...
  server <- function(input, output, session) {...}  # same content...
  shinyApp(ui, server)
}

Shouldn't this be picked up also as a global variable?

@gadenbuie
Copy link
Member

gadenbuie commented Oct 11, 2023

Shouldn't this be picked up also as a global variable?

@MichalLauer It was a reasonable thing to try, for sure! We're using the globals package, specifically globals::globalsOf(). From what I can tell, that package doesn't support utils::globalVariables() in its analysis. They might be open to a feature request, or there may be technical reasons that it's not possible to support globalVariables().

@gadenbuie
Copy link
Member

@schloerke I've confirmed that setting globalsOf(mustExist = FALSE) avoids the false-positive global variable identification in the reprex above. I'm not sure what other impact that would have on shinytest2, however.

@MichalLauer
Copy link

Thanks for the quick update @gadenbuie. I raised a feature request over at HenrikBengtsson/globals, so let's see if this feature is possible.

@schloerke
Copy link
Collaborator

Shouldn't [using `utils::globalVariables("cyl")] be picked up as a global variable?

tl/dr; No. It is not a global value. It is just to stop the warning raised during R CMD check. You should mask the variables with .data$ prefix or setting NULL values at the beginning of the code. Or put your app in a file and supply the file name to AppDriver.


The bug can be traced up to globals:::where(name, envir).

Reprex:

fn <- local({ 
  utils::globalVariables("Species", rlang::current_env()); 
  function() { 
    subset(iris, Species == "setosa")
  }; 
});

globals::globalsOf(fn, environment(fn), mustExist=T)
#> Error in globalsByName(names, envir = envir, mustExist = mustExist) : 
#>   Identified global objects via static code inspection (function (); {; subset(iris, Species == "setosa"); }). Failed to locate global object in the relevant environments: ‘Species’

globals::globalsOf(fn, environment(fn), mustExist=F) %>% str()
#> List of 5
#>  $ {      :.Primitive("{") 
#>  $ subset :function (x, ...)  
#>  $ iris   :'data.frame':        150 obs. of  5 variables:
#>   ..$ Sepal.Length: num [1:150] 5.1 4.9 4.7 4.6 5 5.4 4.6 5 4.4 4.9 ...
#>   ..$ Sepal.Width : num [1:150] 3.5 3 3.2 3.1 3.6 3.9 3.4 3.4 2.9 3.1 ...
#>   ..$ Petal.Length: num [1:150] 1.4 1.4 1.3 1.5 1.4 1.7 1.4 1.5 1.4 1.5 ...
#>   ..$ Petal.Width : num [1:150] 0.2 0.2 0.2 0.2 0.2 0.4 0.3 0.2 0.2 0.1 ...
#>   ..$ Species     : Factor w/ 3 levels "setosa","versicolor",..: 1 1 1 1 1 1 1 1 1 1 ...
#>  $ ==     :function (e1, e2)  
#>  $ Species: NULL
#>  - attr(*, "where")=List of 5
#>   ..$ {      :<environment: base> 
#>   ..$ subset :<environment: base> 
#>   ..$ iris   :<environment: package:datasets> 
#>   .. ..- attr(*, "name")= chr "package:datasets"
#>   .. ..- attr(*, "path")= chr "/Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library/datasets"
#>   ..$ ==     :<environment: base> 
#>   ..$ Species: NULL
#>  - attr(*, "class")= chr [1:2] "Globals" "list"

The important part here is attr(., "where")$Species is NULL. This means that there is no environment found for the variable Species.

withr::with_options(list("globals.debug" = TRUE), globals:::where("Species", envir = environment(fn)))
#> where(‘Species’, where = -1, envir = ‘0x114dfd738’, mode = ‘any’, inherits = TRUE) ...
#> - searching ‘0x114dfd738’: ‘.__global__’
#> - searching ‘R_GlobalEnv’: ‘.__global__’, ‘.First’, ‘.Random.seed’, ..., ‘trace’
#> - searching ‘magrittr’: ‘%!>%’, ‘%<>%’, ‘%>%’, ..., ‘use_series’
#> - searching ‘shiny’: ‘..stacktraceoff..’, ‘..stacktraceon..’, ‘.Depends’, ..., ‘withTags’
#> - searching ‘tools:vscode’: ‘.vsc’, ‘.vsc.attach’, ‘.vsc.browser’, ..., ‘View’
#> - searching ‘stats’: ‘.checkMFClasses’, ‘.getXlevels’, ‘.lm.fit’, ..., ‘xtabs’
#> - searching ‘graphics’: ‘.filled.contour’, ‘abline’, ‘arrows’, ..., ‘yinch’
#> - searching ‘grDevices’: ‘.axisPars’, ‘.clipPath’, ‘.defineGroup’, ..., ‘xyz.coords’
#> - searching ‘utils’: ‘?’, ‘.AtNames’, ‘.DollarNames’, ..., ‘zip’
#> - searching ‘datasets’: ‘ability.cov’, ‘airmiles’, ‘AirPassengers’, ..., ‘WWWusage’
#> - searching ‘methods’: ‘.__C__.environment’, ‘.__C__.externalptr’, ‘.__C__.name’, ..., ‘validSlotNames’
#> - searching ‘Autoloads’: ‘.Autoloaded’, ‘%>%’
#> - searching ‘base’: ‘-’, ‘-.Date’, ‘-.POSIXt’, ..., ‘zapsmall’
#> - failed to locate: NULL
#> where(‘Species’, where = -1, envir = ‘0x114dfd738’, mode = ‘any’, inherits = TRUE) ... DONE
#> 
#> NULL

Looking at the definition for utils::globalVariables()

function (names, package, add = TRUE) 
  registerNames(names, package, ".__global__", add)

it just registers a value of .__global__ in the given environment. We can see this value in the where() debug output above. So asking if Species exists will be FALSE, but the R CMD checking function can look for these registered values and ignore them.


Motivation:

Why are we inspecting the Shiny App object?

We need to have it serialized to load it in a background R process. To properly serialize the app, we need to find all global variables that are used within the function and reconstruct them when running your app in a new R process.

Implied question:

So how do I handle non-standard evaluation within my code?

Context: (TIL) See Henrik's answer from 2018: HenrikBengtsson/globals#35 (comment)

I'd recommend using Garrick's solutions from above or using a third option:
3. Put your app into a file.

  • If you supply a file to AppDriver$new(app_file) rather than an object, then the global variables are not used.

I understand there is no clean solution, but non-standard evaluation makes static code checking impossible.

If a file is used to init the AppDriver, then the file path is provided to the background, which does not require any inspecting of global variables as the app file should work on it's own.

@MichalLauer
Copy link

MichalLauer commented Oct 11, 2023

Thanks for the detailed answer @schloerke! So the final resolution is that shinytest2 will keep using globalsOf(mustExist = TRUE) and I should update my code with either cyl <- NULL or .data$cyl. Is that correct?

Edit: Would it be maybe possible to control the behavior of the mustExist parameter via a global variable? (e.g., to call Sys.setenv(SHINYTEST2_GLOBS_MUST_EXIST = F) )

@gadenbuie
Copy link
Member

gadenbuie commented Oct 11, 2023

I should update my code with either cyl <- NULL or .data$cyl. Is that correct?

@MichalLauer Using .data$cyl is definitely the preferred approach, especially if you're working on an R package. It's best practices in R packages and generally preferred over using utils::globalVariables().

@schloerke thanks for all of the context. I think there's an argument to be made that we can use mustExist = FALSE. With this set, we'll serialize all the global variables that could possibly be serialized, and if we can't find a variable the app might work or might fail to run. I'm not sure exactly what failure mode would surface, but it feels like moving forward in the face of possible falsely identified global variables would generally allow more runnable apps to run.

@schloerke
Copy link
Collaborator

I'd rather not do a sysenv if possible. I'd be ok allowing the user to opt-in to 🦶🔫 via an R option. (😆)

@gadenbuie How does updating

globals <- globals::globalsOf(server, envir = environment(server), recursive = TRUE)
to be something similar to:

  globals <- globals::globalsOf(server, envir = environment(server), recursive = TRUE, mustExist = isTRUE(getOption("shinytest2.app_save.globals_must_exist", TRUE)))

sound to you?

@schloerke
Copy link
Collaborator

We could also wrap the line in the trycatch and provide a better error and notes on how to set the shinytest2 option.

I think there's an argument to be made that we can use mustExist = FALSE.

I'd like to default to TRUE and require the user to opt-in to using FALSE.

The headache of trying to understand why a value doesn't exist is no fun and hard to chase. I'd rather have the error message and notes on how to disable it with an option

@gadenbuie
Copy link
Member

@schloerke I'm all for making it a user opt-in and giving help in the instructions. Given where and how Shiny apps are tested, I think we should consider using envvars rather than an R option. They're easier to set in CI like GitHub actions and they're passed from the parent process to child processes, whereas R options are not.

options(foo = "bar")
Sys.setenv(foo = "baz")

r <- callr::r_bg(function() {
  list(
    opt = getOption("foo"),
    env = Sys.getenv("foo")
  )
})

r$wait()
r$get_result()
#> $opt
#> NULL
#> 
#> $env
#> [1] "baz"

@schloerke
Copy link
Collaborator

The app is being saved during local testing as well. So the the CI environment variable wont exist locally and there should be no different behavior for the two testing locations.

I believe the option should be set within the test file or shinytest2 setup file. We can discuss offline about details.

@gadenbuie
Copy link
Member

@schloerke and I talked to today and agreed to create an option, named shinytest2.app_save.globals_must_exist that could be set before calling AppDriver$new(), either in the test file directly or globally in the tests/testthat/setup-shinytest2.R file.

@MichalLauer
Copy link

Hello guys, thank you very much! Really appriciate the work you are doing. Looking forward to a new release :)

@MichalLauer
Copy link

Hello,
Is there an ETA for a version that will contain this change? I should have some spare time in January, and if the changes are straightforward, I can help with the implementation.

Thanks, and Merry Christmas!🎄

@MichalLauer
Copy link

MichalLauer commented Feb 2, 2024

Hello @gadenbuie @schloerke,

any news regarding the implementation? Can I be of some help? :)

Edit: will try to create a PR and we'll see if it's the correct implementation
Edit2: unless I'm doing something wrong, I can't create a PR so I'll leave this up to you:)

@gadenbuie
Copy link
Member

gadenbuie commented Apr 12, 2024

Here's a small reprex based on the PR to DT (rstudio/DT#1136)

library(shiny)

app_run <- function() {
  server <- function(input, output, session) {
    two <- function() one()
    one <- function() "first"
  }

  shinyApp(fluidPage(), server)
}

app <- app_run()
shinytest2::AppDriver$new(app)
#> Caused by error in `globalsByName()`:
#> ! Identified global objects via static code inspection (function (input, output, session); {; two <- function() one(); one <- function() "first"; }). Failed to locate global object in the relevant environments: 'one'

@gadenbuie
Copy link
Member

The "failed to locate" error is thrown by globals::globalsByName() (code here)

      globals[name] <- list(NULL)
      where[name] <- list(NULL)
      if (mustExist) {
        stop(sprintf("Failed to locate global object in the relevant environments: %s", sQuote(name))) #nolint
      }

We call globals::globalsOf() (which calls globalsByName()) here

globals <- globals::globalsOf(server, envir = environment(server), recursive = TRUE)

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

Successfully merging a pull request may close this issue.

4 participants