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

Allow connecting to an existing Shiny instance #348

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

warnes
Copy link

@warnes warnes commented Aug 18, 2020

One of my shinytest scripts triggered an error in the app that did not occur when the app ran interactively, so I needed to use the debugger on the app as the test script executed.

A few additional lines of code allows running the shiny app in one RStudio instance, and the shinytest script in another instance by providing an additional (optional) url argument.

This PR is independent of #324. and should resolve #85.

@hadley
Copy link
Member

hadley commented Aug 18, 2020

Are you sure that this doesn't already work? It's not documented, but I think you can supply a url to the path argument — at least, I think that that's what this code allows for: https://github.com/rstudio/shinytest/blob/master/R/initialize.R#L24-L26

@warnes
Copy link
Author

warnes commented Aug 18, 2020

Those lines suffice to get the PhantomJS process to connect to the shiny app, but there were 2 issues in the ShinyDriver:

  1. The code assumes that it can use private$path to locate the directory where the snapshot files should go, and
    2 The code didn't check if private$shinyProcess existed.

Both led to errors when path was a URL.

Only 10 lines of code needed to fix these. Adding a separate url parameter fixed the first issue, and a little bit of guard code fixed the second.

The rest of the patch is:

  • Documenting the new parameter and behavior for the man page and "Details" vignette, and
  • Adding a test case.

@@ -4,7 +4,7 @@

sd_initialize <- function(self, private, path, loadTimeout, checkNames,
debug, phantomTimeout, seed, cleanLogs,
shinyOptions) {
shinyOptions, url=NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please put spaces around =?

@@ -21,9 +21,13 @@ sd_initialize <- function(self, private, path, loadTimeout, checkNames,
self$logEvent("Getting PhantomJS port")
private$phantomPort <- get_phantomPort(timeout = phantomTimeout)

if (grepl("^http(s?)://", path)) {
private$setShinyUrl(path)
if(!dir_exists(path))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please follow https://style.tidyverse.org here?

  • Space after if
  • {} around stop
  • <- instead of =

#' headless browser that can be used to simulate user actions. This provides
#' a full simulation of a Shiny app so that you can test user interactions
#' with a live app.
#'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove this empty line?

#' a full simulation of a Shiny app so that you can test user interactions
#' with a live app.
#'
#' This class connects a `phantom.js` headless browser to a shiny app to enable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#' This class connects a `phantom.js` headless browser to a shiny app to enable
#' This class connects a `phantom.js` headless browser to a Shiny app to enable

#' with a live app.
#'
#' This class connects a `phantom.js` headless browser to a shiny app to enable
#' testing the effect of user interactions with a live app. Specify `url` to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the path clause should come first since that's what most people will do.

@@ -41,14 +44,15 @@ ShinyDriver <- R6Class(
#' @param cleanLogs Whether to remove the stdout and stderr logs when the
#' Shiny process object is garbage collected.
#' @param shinyOptions A list of options to pass to [shiny::runApp()].
#' @param url Optional URL where the shiny app is executing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not provided, ShinyDriver will automatically start a shiny instance.

Comment on lines +8 to +10
client <- ShinyDriver$new(test_path("apps/outputs"),
url = server$getUrl()
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
client <- ShinyDriver$new(test_path("apps/outputs"),
url = server$getUrl()
)
client <- ShinyDriver$new(test_path("apps/outputs"), url = server$getUrl())

client$waitForShiny()
expect_equal(client$getValue("i"), "2")

client$stop()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't call these explicitly in any other tests, so I think you should remove them here, unless there is something special going on.

@@ -269,6 +269,32 @@ app <- ShinyDriver$new("path/to/app")

The rest of the test script can be run unchanged.

## Debugging app issues

IF you need to debug an issue in the application that is triggered by a test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
IF you need to debug an issue in the application that is triggered by a test
If you need to debug an issue in the application that is triggered by a test

Comment on lines +289 to +291
client <- ShinyDriver$new(test_path("apps/outputs"),
url = "http://localhost:4040"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
client <- ShinyDriver$new(test_path("apps/outputs"),
url = "http://localhost:4040"
)
client <- ShinyDriver$new("path/to/app"), url = "http://localhost:4040")

@bersbersbers
Copy link

bersbersbers commented Nov 18, 2021

This would be awesome to have to be able to remotely test a deployed app. I tried this by applying the below patch. Below my experience. TLDR: It seems to work, but proxies and HTTP authentication seem to cause problems. I'd be glad about a workaround particularly to the latter issue.

  • On a system behind a proxy, shinytest::ShinyDriver$new(url = "http://www.example.com/") simply stalls. This will be due to
    https://github.com/rstudio/webdriver/blob/13b76674b9bb5ba94c0267ab56f9654396273e0d/R/run-phantomjs.R#L66-L70
    I guess.

  • On a system not behind a proxy, I get

    Error: Shiny app did not load in 5000ms.
    S/I 12:10:32.00 window.shinytest loaded
    S/I 12:10:32.00 jQuery not loaded yet
    Run rlang::last_error() to see where the error occurred.

    which I can imagine is expected.

  • shinytest::ShinyDriver$new(url = "https://vnijs.shinyapps.io/radiant/") returns a <ShinyDriver> instance.

  • My app being behind HTTP authentication, I tried url = "https://user:pass@....", however, shinytest:::parse_url rejects that as an invalid URL.

diff --git a/R/initialize.R b/R/initialize.R
index 780ffdb..fc3f903 100644
--- a/R/initialize.R
+++ b/R/initialize.R
@@ -7 +7 @@ sd_initialize <- function(self, private, path, loadTimeout, checkNames,
-                          shinyOptions, renderArgs, options) {
+                          shinyOptions, renderArgs, options, url = NULL) {
@@ -28,2 +28,7 @@ sd_initialize <- function(self, private, path, loadTimeout, checkNames,
-  if (grepl("^http(s?)://", path)) {
-    private$setShinyUrl(path)
+  if(!dir_exists(path))
+    stop("No such directory: ", path)
+  private$path <- path
+
+  if(!is.null(url)) {
+    "!DEBUG setting URL provided by the user"
+    private$setShinyUrl(url)
@@ -250 +255 @@ sd_finalize <- function(self, private) {
-  if (isTRUE(private$cleanLogs)) {
+  if (!is.null(private$shinyProcess) && isTRUE(private$cleanLogs)) {
diff --git a/R/shiny-driver.R b/R/shiny-driver.R
index cf36003..518baee 100644
--- a/R/shiny-driver.R
+++ b/R/shiny-driver.R
@@ -50 +50 @@ ShinyDriver <- R6Class(
-      shinyOptions = list(), renderArgs = NULL, options = list())
+      shinyOptions = list(), renderArgs = NULL, options = list(), url = NULL)
@@ -54 +54 @@ ShinyDriver <- R6Class(
-        shinyOptions = shinyOptions, renderArgs = renderArgs, options = options)
+        shinyOptions = shinyOptions, renderArgs = renderArgs, options = options, url = url)

@bersbersbers
Copy link

bersbersbers commented Nov 18, 2021

I made shinytest understand HTTP auth in the below patch, but now (HTTP credentials, not behind proxy) I am at the same point as behind a proxy (stalling).

Are there any known restrictions with HTTP auth in webdriver? (Edit: I could isolate this issue to being an SSL error in webdriver related to specific app - the code below is working fine apart from that.)

Sharing this piece of code either way:

diff --git a/R/initialize.R b/R/initialize.R
index 780ffdb..34cc46e 100644
--- a/R/initialize.R
+++ b/R/initialize.R
@@ -7 +7 @@ sd_initialize <- function(self, private, path, loadTimeout, checkNames,
-                          shinyOptions, renderArgs, options) {
+                          shinyOptions, renderArgs, options, url = NULL) {
@@ -28,2 +28,7 @@ sd_initialize <- function(self, private, path, loadTimeout, checkNames,
-  if (grepl("^http(s?)://", path)) {
-    private$setShinyUrl(path)
+  if(!dir_exists(path))
+    stop("No such directory: ", path)
+  private$path <- path
+
+  if(!is.null(url)) {
+    "!DEBUG setting URL provided by the user"
+    private$setShinyUrl(url)
@@ -204 +209,7 @@ sd_getShinyUrl <- function(self, private) {
-    private$shinyUrlProtocol, "://", private$shinyUrlHost,
+    private$shinyUrlProtocol, "://",
+    if (!is.null(private$shinyUrlUser)) paste0(
+      private$shinyUrlUser,
+      if (!is.null(private$shinyUrlPass)) paste0(":", private$shinyUrlPass),
+      "@"
+    ),
+    private$shinyUrlHost,
@@ -225,0 +237,2 @@ sd_setShinyUrl <- function(self, private, url) {
+  private$shinyUrlUser     <- res$user
+  private$shinyUrlPass     <- res$pass
@@ -250 +263 @@ sd_finalize <- function(self, private) {
-  if (isTRUE(private$cleanLogs)) {
+  if (!is.null(private$shinyProcess) && isTRUE(private$cleanLogs)) {
diff --git a/R/shiny-driver.R b/R/shiny-driver.R
index cf36003..152a9e4 100644
--- a/R/shiny-driver.R
+++ b/R/shiny-driver.R
@@ -50 +50 @@ ShinyDriver <- R6Class(
-      shinyOptions = list(), renderArgs = NULL, options = list())
+      shinyOptions = list(), renderArgs = NULL, options = list(), url = NULL)
@@ -54 +54 @@ ShinyDriver <- R6Class(
-        shinyOptions = shinyOptions, renderArgs = renderArgs, options = options)
+        shinyOptions = shinyOptions, renderArgs = renderArgs, options = options, url = url)
@@ -466,0 +467,2 @@ ShinyDriver <- R6Class(
+    shinyUrlUser = NULL,
+    shinyUrlPass = NULL,
diff --git a/R/utils.R b/R/utils.R
index d91c2ae..cecb958 100644
--- a/R/utils.R
+++ b/R/utils.R
@@ -98 +98 @@ parse_url <- function(url) {
-  res <- regexpr("^(?<protocol>https?)://(?<host>[^:/]+)(:(?<port>\\d+))?(?<path>/.*)?$", url, perl = TRUE)
+  res <- regexpr("^(?<protocol>https?)://((?<user>[^:]*)(:(?<pass>.*))?@)?(?<host>[^:/]+)(:(?<port>\\d+))?(?<path>/.*)?$", url, perl = TRUE)
@@ -112,0 +113,2 @@ parse_url <- function(url) {
+    user     = get_piece("user"),
+    pass     = get_piece("pass"),

@bersbersbers
Copy link

I would love to take this forward as this is working great for me. @warnes, do you mind if I start a new PR based on yours and the review comments? @hadley, do you think it is likely that this functionality will be merged if the PR is according to all standards?

@warnes
Copy link
Author

warnes commented Feb 14, 2022

I would love to take this forward as this is working great for me. @warnes, do you mind if I start a new PR based on yours and the review comments? @hadley, do you think it is likely that this functionality will be merged if the PR is according to all standards?

You're welcome to do so!

@hadley
Copy link
Member

hadley commented Feb 14, 2022

I think the broader environment has changed since I last looked at this PR. @schloerke can comment if it would still be useful to finish off.

@schloerke
Copy link
Contributor

@bersbersbers I can connect to a local shiny instance already.

Reprex:

# Terminal 1
shiny::runApp(
  system.file("examples/01_hello", package = "shiny"), 
  test.mode=TRUE
)
#> Running application in test mode.
#> 
#> Listening on http://127.0.0.1:6427
# Terminal 2
app <- ShinyDriver$new("http://127.0.0.1:6427")
app$getAllValues()$input$bins
#> [1] 30
app$setInputs(bins = 20)
app$getAllValues()$input$bins
#> [1] 20

@bersbersbers I am hesitant to support adding another parameter when path could used for the url value. Reasoning... If url is used, I don't readily see a reason for supplying path other than to make the error checks happy.

Adding support for the user/pass in the url string is very reasonable. A small PR for this logic would be great!

An item to remember... the app MUST be started in test mode (Ex: shiny::runApp(test.mode = TRUE)). Having options(shiny.testmode = TRUE) in your app.R is too late. I'm guessing this is why the shinyapps.io is failing as no one has fine tune control over how the app is launched.


Debugging a live app will not work as intended as a new session will be started. Other than the user/pass support, this PR feels like this debugging effort is being forced. Instead, I'd like to push effort towards {shinytest2}. I'll make the repo public later this week.

Mainly, {shinytest2} will have a method app$view() to view the live application without starting a new session and will be displayed in your local Chrome browser via {chromote}.

Life should be much better on the {shinytest2} side of the fence.

@bersbersbers
Copy link

bersbersbers commented Feb 14, 2022

@bersbersbers I am hesitant to support adding another parameter when path could used for the url value. Reasoning... If url is used, I don't readily see a reason for supplying path other than to make the error checks happy.

I may be mistaken, but I think @warnes has given one reason in #348 (comment), which is that if path is a URL, shinytest does not find the folder with expected outputs to compare against to store the current output to (so besides a URL, if not a path it requires at least the name of the test.). So maybe there is a better way to run tests and compare outputs on a remote instances, but I haven't found it yet.

(My explanations above may not be 100% accurate, but I think you get the point: when connecting to a remote instance, you still need a local reference for where the test output should go.)


I am aware that the to-be-tested app has to be started in test mode for most functionality, although do note that is not stricly required: I have, in parallel, started working on making shinytest use fixed wait times for my tests and store only screenshots (no JSON snapshots). With this approach, I am able to test an app that is not in test mode at all (+/- all the other issues, SSL certs, proxy, HTTP auth etc, but a few examples are working already).


Debugging a live app will not work as intended as a new session will be started.

(I think) I agree, but that is not my aim anyway. My aim is to make sure a deployed Shiny instance is fully operational each day and does not run into issues such as network connectivity, RAM exhaustion, running out of disk space, all those things that may happen to a rarely used server at some point or another. (We have had such cases twice in the past month, and we hope to be able to fix things by increasing the VM's RAM, but it is still unclear if that works.)

@bersbersbers
Copy link

bersbersbers commented Feb 15, 2022

To comment on this reprex:

# Terminal 1
shiny::runApp(
  system.file("examples/01_hello", package = "shiny"), 
  test.mode=TRUE
)
#> Running application in test mode.
#> 
#> Listening on http://127.0.0.1:6427
# Terminal 2
app <- ShinyDriver$new("http://127.0.0.1:6427")
app$getAllValues()$input$bins
#> [1] 30
app$setInputs(bins = 20)
app$getAllValues()$input$bins
#> [1] 20

I agree this works, but now run app$getAppDir() or app$snapshot():

> app <- shinytest::ShinyDriver$new("http://127.0.0.1:5077")
> app$getAllValues()$input$bins
[1] 30
> app$setInputs(bins = 20)
> app$getAllValues()$input$bins
[1] 20
> app$snapshotInit("mytest")
> app$snapshot()
Error in dir.exists(x) : invalid filename argument
> app$getAppDir()
Error in dir.exists(x) : invalid filename argument

@bersbersbers
Copy link

For the record, I'd like to back the need to connect to an existing Shiny instance using rstudio/shiny#3606. That issue exists, as far as I can tell, only when the app is deployed on a Shiny Server, and not when run locally - yet there is no way to run a test suite on that deployed instance. I only detected that bug by doing (unrelated) work on that app using Python/Selenium. I don't really like to transfer all my shinytests to Selenium for that, so I will (have to) continue working using my shinytest fork based on this PR.

@schloerke
Copy link
Contributor

@bersbersbers {shinytest} is entering maintenance mode as it is being superseded by {shinytest2}.

If possible, please transfer your tests from {shinytest} to {shinytest2}. shinytest2::migrate_from_shinytest(app_dir) can help automate the process from {shinytest} tests to {shinytest2} tests.

If you run into any bugs using a url (AppDriver$new(app_dir=URL)) for your app location or during your migration, please file an issue in {shinytest2}.

@bersbersbers
Copy link

shinytest2::AppDriver$new(app_dir=URL) is actually looking great so far, thanks! Looking forward to seeing that on CRAN.

@CLAassistant
Copy link

CLAassistant commented Nov 8, 2023

CLA assistant check
All committers have signed the CLA.

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 this pull request may close these issues.

Idea: To debug a failing test, be able to "browse()" into the app
5 participants