From 2a5bfb6e9853725ee862c21afe592c2faf22d1cc Mon Sep 17 00:00:00 2001 From: Matthew Lynch Date: Wed, 26 Jul 2023 11:43:24 -0500 Subject: [PATCH 1/6] don't restrict shinyapps.io from previous deployment check in deploymentTarget --- R/deploymentTarget.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/deploymentTarget.R b/R/deploymentTarget.R index 82133b7b..4e2b9690 100644 --- a/R/deploymentTarget.R +++ b/R/deploymentTarget.R @@ -26,7 +26,7 @@ deploymentTarget <- function(recordPath = ".", } appId <- NULL - if (!isCloudServer(fullAccount$server)) { + if (!isPositCloudServer(fullAccount$server)) { # Have we previously deployed elsewhere? We can't do this on cloud # because it assigns random app names (see #808 for details). existing <- applications(fullAccount$name, fullAccount$server) From 09555d256b495711e9f02e1eb15d3553931cadd3 Mon Sep 17 00:00:00 2001 From: Matthew Lynch Date: Wed, 26 Jul 2023 14:55:20 -0500 Subject: [PATCH 2/6] add tests for shinyapps.io deploymentTarget and NEWS.md entry for 932 bugfix --- NEWS.md | 3 ++ tests/testthat/test-deploymentTarget.R | 70 ++++++++++++++------------ 2 files changed, 40 insertions(+), 33 deletions(-) diff --git a/NEWS.md b/NEWS.md index 42a12afa..c81ec11d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,8 @@ # rsconnect (development version) +* Fixed redeployments to shinyapps.io where `appName` is provided, but no local + record of the deployment exists. (#932) + * `deployApp()` and `writeManifest()` now error if your library and `renv.lock` are out-of-sync. Previously it always used what was defined in the `renv.lock` but that was (a) slow and (b) could lead to different results than what you diff --git a/tests/testthat/test-deploymentTarget.R b/tests/testthat/test-deploymentTarget.R index 9f242b3c..c686cad7 100644 --- a/tests/testthat/test-deploymentTarget.R +++ b/tests/testthat/test-deploymentTarget.R @@ -187,42 +187,46 @@ test_that("default title is the empty string", { }) test_that("can find existing application on server & use it", { - local_temp_config() - addTestServer() - addTestAccount("ron") - local_mocked_bindings( - applications = function(...) data.frame( - name = "my_app", - id = 123, - url = "http://example.com/test", - stringsAsFactors = FALSE - ), - shouldUpdateApp = function(...) TRUE - ) - - app_dir <- withr::local_tempdir() - target <- deploymentTarget(app_dir, appName = "my_app") - expect_equal(target$appId, 123) + for (server in c("example.com", "shinyapps.io")) { + local_temp_config() + addTestServer() + addTestAccount("ron", server = server) + local_mocked_bindings( + applications = function(...) data.frame( + name = "my_app", + id = 123, + url = "http://example.com/test", + stringsAsFactors = FALSE + ), + shouldUpdateApp = function(...) TRUE + ) + + app_dir <- withr::local_tempdir() + target <- deploymentTarget(app_dir, appName = "my_app", server = server) + expect_equal(target$appId, 123) + } }) test_that("can find existing application on server & not use it", { - local_temp_config() - addTestServer() - addTestAccount("ron") - local_mocked_bindings( - applications = function(...) data.frame( - name = "my_app", - id = 123, - url = "http://example.com/test", - stringsAsFactors = FALSE - ), - shouldUpdateApp = function(...) FALSE - ) - - app_dir <- withr::local_tempdir() - target <- deploymentTarget(app_dir, appName = "my_app") - expect_equal(target$appName, "my_app-1") - expect_equal(target$appId, NULL) + for (server in c("example.com", "shinyapps.io")) { + local_temp_config() + addTestServer() + addTestAccount("ron", server = server) + local_mocked_bindings( + applications = function(...) data.frame( + name = "my_app", + id = 123, + url = "http://example.com/test", + stringsAsFactors = FALSE + ), + shouldUpdateApp = function(...) FALSE + ) + + app_dir <- withr::local_tempdir() + target <- deploymentTarget(app_dir, appName = "my_app", server = server) + expect_equal(target$appName, "my_app-1") + expect_equal(target$appId, NULL) + } }) # defaultAppName ---------------------------------------------------------- From 78ddd1dddcceeb666a74faba2346d030a5780895 Mon Sep 17 00:00:00 2001 From: Matthew Lynch Date: Wed, 26 Jul 2023 14:57:03 -0500 Subject: [PATCH 3/6] add tests for shinyapps.io deploymentTarget and NEWS.md entry for 932 bugfix --- NEWS.md | 8 --- tests/testthat/test-deploymentTarget.R | 70 ++++++++++++-------------- 2 files changed, 33 insertions(+), 45 deletions(-) diff --git a/NEWS.md b/NEWS.md index c81ec11d..16804010 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,13 +1,5 @@ # rsconnect (development version) -* Fixed redeployments to shinyapps.io where `appName` is provided, but no local - record of the deployment exists. (#932) - -* `deployApp()` and `writeManifest()` now error if your library and `renv.lock` - are out-of-sync. Previously it always used what was defined in the `renv.lock` - but that was (a) slow and (b) could lead to different results than what you - see when running locally (#930). - * Deploying from an renv project includes the `renv.lock` in the bundle. A manifest created for an renv project references the `renv.lock` in the `manifest.json`. (#926) diff --git a/tests/testthat/test-deploymentTarget.R b/tests/testthat/test-deploymentTarget.R index c686cad7..9f242b3c 100644 --- a/tests/testthat/test-deploymentTarget.R +++ b/tests/testthat/test-deploymentTarget.R @@ -187,46 +187,42 @@ test_that("default title is the empty string", { }) test_that("can find existing application on server & use it", { - for (server in c("example.com", "shinyapps.io")) { - local_temp_config() - addTestServer() - addTestAccount("ron", server = server) - local_mocked_bindings( - applications = function(...) data.frame( - name = "my_app", - id = 123, - url = "http://example.com/test", - stringsAsFactors = FALSE - ), - shouldUpdateApp = function(...) TRUE - ) - - app_dir <- withr::local_tempdir() - target <- deploymentTarget(app_dir, appName = "my_app", server = server) - expect_equal(target$appId, 123) - } + local_temp_config() + addTestServer() + addTestAccount("ron") + local_mocked_bindings( + applications = function(...) data.frame( + name = "my_app", + id = 123, + url = "http://example.com/test", + stringsAsFactors = FALSE + ), + shouldUpdateApp = function(...) TRUE + ) + + app_dir <- withr::local_tempdir() + target <- deploymentTarget(app_dir, appName = "my_app") + expect_equal(target$appId, 123) }) test_that("can find existing application on server & not use it", { - for (server in c("example.com", "shinyapps.io")) { - local_temp_config() - addTestServer() - addTestAccount("ron", server = server) - local_mocked_bindings( - applications = function(...) data.frame( - name = "my_app", - id = 123, - url = "http://example.com/test", - stringsAsFactors = FALSE - ), - shouldUpdateApp = function(...) FALSE - ) - - app_dir <- withr::local_tempdir() - target <- deploymentTarget(app_dir, appName = "my_app", server = server) - expect_equal(target$appName, "my_app-1") - expect_equal(target$appId, NULL) - } + local_temp_config() + addTestServer() + addTestAccount("ron") + local_mocked_bindings( + applications = function(...) data.frame( + name = "my_app", + id = 123, + url = "http://example.com/test", + stringsAsFactors = FALSE + ), + shouldUpdateApp = function(...) FALSE + ) + + app_dir <- withr::local_tempdir() + target <- deploymentTarget(app_dir, appName = "my_app") + expect_equal(target$appName, "my_app-1") + expect_equal(target$appId, NULL) }) # defaultAppName ---------------------------------------------------------- From 7944212833f47cc53be05af6d11f9b125f1bfaeb Mon Sep 17 00:00:00 2001 From: Matthew Lynch Date: Wed, 26 Jul 2023 14:58:38 -0500 Subject: [PATCH 4/6] Revert "add tests for shinyapps.io deploymentTarget and NEWS.md entry for 932 bugfix" This reverts commit 78ddd1dddcceeb666a74faba2346d030a5780895. --- NEWS.md | 8 +++ tests/testthat/test-deploymentTarget.R | 70 ++++++++++++++------------ 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/NEWS.md b/NEWS.md index 16804010..c81ec11d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,13 @@ # rsconnect (development version) +* Fixed redeployments to shinyapps.io where `appName` is provided, but no local + record of the deployment exists. (#932) + +* `deployApp()` and `writeManifest()` now error if your library and `renv.lock` + are out-of-sync. Previously it always used what was defined in the `renv.lock` + but that was (a) slow and (b) could lead to different results than what you + see when running locally (#930). + * Deploying from an renv project includes the `renv.lock` in the bundle. A manifest created for an renv project references the `renv.lock` in the `manifest.json`. (#926) diff --git a/tests/testthat/test-deploymentTarget.R b/tests/testthat/test-deploymentTarget.R index 9f242b3c..c686cad7 100644 --- a/tests/testthat/test-deploymentTarget.R +++ b/tests/testthat/test-deploymentTarget.R @@ -187,42 +187,46 @@ test_that("default title is the empty string", { }) test_that("can find existing application on server & use it", { - local_temp_config() - addTestServer() - addTestAccount("ron") - local_mocked_bindings( - applications = function(...) data.frame( - name = "my_app", - id = 123, - url = "http://example.com/test", - stringsAsFactors = FALSE - ), - shouldUpdateApp = function(...) TRUE - ) - - app_dir <- withr::local_tempdir() - target <- deploymentTarget(app_dir, appName = "my_app") - expect_equal(target$appId, 123) + for (server in c("example.com", "shinyapps.io")) { + local_temp_config() + addTestServer() + addTestAccount("ron", server = server) + local_mocked_bindings( + applications = function(...) data.frame( + name = "my_app", + id = 123, + url = "http://example.com/test", + stringsAsFactors = FALSE + ), + shouldUpdateApp = function(...) TRUE + ) + + app_dir <- withr::local_tempdir() + target <- deploymentTarget(app_dir, appName = "my_app", server = server) + expect_equal(target$appId, 123) + } }) test_that("can find existing application on server & not use it", { - local_temp_config() - addTestServer() - addTestAccount("ron") - local_mocked_bindings( - applications = function(...) data.frame( - name = "my_app", - id = 123, - url = "http://example.com/test", - stringsAsFactors = FALSE - ), - shouldUpdateApp = function(...) FALSE - ) - - app_dir <- withr::local_tempdir() - target <- deploymentTarget(app_dir, appName = "my_app") - expect_equal(target$appName, "my_app-1") - expect_equal(target$appId, NULL) + for (server in c("example.com", "shinyapps.io")) { + local_temp_config() + addTestServer() + addTestAccount("ron", server = server) + local_mocked_bindings( + applications = function(...) data.frame( + name = "my_app", + id = 123, + url = "http://example.com/test", + stringsAsFactors = FALSE + ), + shouldUpdateApp = function(...) FALSE + ) + + app_dir <- withr::local_tempdir() + target <- deploymentTarget(app_dir, appName = "my_app", server = server) + expect_equal(target$appName, "my_app-1") + expect_equal(target$appId, NULL) + } }) # defaultAppName ---------------------------------------------------------- From 6f4ff2395111c08f27de8934c91a7f62b24711bc Mon Sep 17 00:00:00 2001 From: Matthew Lynch Date: Wed, 26 Jul 2023 17:27:39 -0500 Subject: [PATCH 5/6] clearer separation between shinyapps/connect tests --- tests/testthat/test-deploymentTarget.R | 86 +++++++++++++++----------- 1 file changed, 49 insertions(+), 37 deletions(-) diff --git a/tests/testthat/test-deploymentTarget.R b/tests/testthat/test-deploymentTarget.R index c686cad7..966d3dbb 100644 --- a/tests/testthat/test-deploymentTarget.R +++ b/tests/testthat/test-deploymentTarget.R @@ -186,47 +186,59 @@ test_that("default title is the empty string", { expect_equal(target$appTitle, "") }) +confirm_existing_application_used <- function(server) { + local_temp_config() + addTestServer() + addTestAccount("ron", server = server) + local_mocked_bindings( + applications = function(...) data.frame( + name = "my_app", + id = 123, + url = "http://example.com/test", + stringsAsFactors = FALSE + ), + shouldUpdateApp = function(...) TRUE + ) + + app_dir <- withr::local_tempdir() + target <- deploymentTarget(app_dir, appName = "my_app", server = server) + expect_equal(target$appId, 123) +} + test_that("can find existing application on server & use it", { - for (server in c("example.com", "shinyapps.io")) { - local_temp_config() - addTestServer() - addTestAccount("ron", server = server) - local_mocked_bindings( - applications = function(...) data.frame( - name = "my_app", - id = 123, - url = "http://example.com/test", - stringsAsFactors = FALSE - ), - shouldUpdateApp = function(...) TRUE - ) - - app_dir <- withr::local_tempdir() - target <- deploymentTarget(app_dir, appName = "my_app", server = server) - expect_equal(target$appId, 123) - } + confirm_existing_application_used("example.com") +}) + +test_that("can find existing application on shinyapps.io & use it", { + confirm_existing_application_used("shinyapps.io") }) +confirm_existing_application_not_used <- function(server) { + local_temp_config() + addTestServer() + addTestAccount("ron", server = server) + local_mocked_bindings( + applications = function(...) data.frame( + name = "my_app", + id = 123, + url = "http://example.com/test", + stringsAsFactors = FALSE + ), + shouldUpdateApp = function(...) FALSE + ) + + app_dir <- withr::local_tempdir() + target <- deploymentTarget(app_dir, appName = "my_app", server = server) + expect_equal(target$appName, "my_app-1") + expect_equal(target$appId, NULL) +} + test_that("can find existing application on server & not use it", { - for (server in c("example.com", "shinyapps.io")) { - local_temp_config() - addTestServer() - addTestAccount("ron", server = server) - local_mocked_bindings( - applications = function(...) data.frame( - name = "my_app", - id = 123, - url = "http://example.com/test", - stringsAsFactors = FALSE - ), - shouldUpdateApp = function(...) FALSE - ) - - app_dir <- withr::local_tempdir() - target <- deploymentTarget(app_dir, appName = "my_app", server = server) - expect_equal(target$appName, "my_app-1") - expect_equal(target$appId, NULL) - } + confirm_existing_application_not_used("example.com") +}) + +test_that("can find existing application on shinyapps.io & not use it", { + confirm_existing_application_not_used("shinyapps.io") }) # defaultAppName ---------------------------------------------------------- From daa426e9c3cbcb2f55c77687b2f1bcfc73da942a Mon Sep 17 00:00:00 2001 From: Matthew Lynch Date: Wed, 26 Jul 2023 18:27:04 -0500 Subject: [PATCH 6/6] shorten test helper function names --- tests/testthat/test-deploymentTarget.R | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/testthat/test-deploymentTarget.R b/tests/testthat/test-deploymentTarget.R index 966d3dbb..89093c05 100644 --- a/tests/testthat/test-deploymentTarget.R +++ b/tests/testthat/test-deploymentTarget.R @@ -186,7 +186,7 @@ test_that("default title is the empty string", { expect_equal(target$appTitle, "") }) -confirm_existing_application_used <- function(server) { +confirm_existing_app_used <- function(server) { local_temp_config() addTestServer() addTestAccount("ron", server = server) @@ -206,14 +206,14 @@ confirm_existing_application_used <- function(server) { } test_that("can find existing application on server & use it", { - confirm_existing_application_used("example.com") + confirm_existing_app_used("example.com") }) test_that("can find existing application on shinyapps.io & use it", { - confirm_existing_application_used("shinyapps.io") + confirm_existing_app_used("shinyapps.io") }) -confirm_existing_application_not_used <- function(server) { +confirm_existing_app_not_used <- function(server) { local_temp_config() addTestServer() addTestAccount("ron", server = server) @@ -234,11 +234,11 @@ confirm_existing_application_not_used <- function(server) { } test_that("can find existing application on server & not use it", { - confirm_existing_application_not_used("example.com") + confirm_existing_app_not_used("example.com") }) test_that("can find existing application on shinyapps.io & not use it", { - confirm_existing_application_not_used("shinyapps.io") + confirm_existing_app_not_used("shinyapps.io") }) # defaultAppName ----------------------------------------------------------