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.

Sign up for GitHub

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

don't restrict shinyapps.io from previous deployment check in deploymentTarget #933

Merged
merged 6 commits into from
Jul 27, 2023
Merged

don't restrict shinyapps.io from previous deployment check in deploymentTarget #933

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a5bfb6
don't restrict shinyapps.io from previous deployment check in deploym…
mslynch Jul 26, 2023
09555d2
add tests for shinyapps.io deploymentTarget and NEWS.md entry for 932…
mslynch Jul 26, 2023
78ddd1d
add tests for shinyapps.io deploymentTarget and NEWS.md entry for 932…
mslynch Jul 26, 2023
7944212
Revert "add tests for shinyapps.io deploymentTarget and NEWS.md entry…
mslynch Jul 26, 2023
6f4ff23
clearer separation between shinyapps/connect tests
mslynch Jul 26, 2023
daa426e
shorten test helper function names
mslynch Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion R/deploymentTarget.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
70 changes: 37 additions & 33 deletions tests/testthat/test-deploymentTarget.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is best done with a helper function and two invocations of that test function -- that will help us understand which condition is failing. Something like:

confirm_existing_application_use <- function(server) {
    # innards of loop
}
test_that("can find existing application on server & use it", {
  confirm_existing_application_use("example.com")
})
test_that("can find existing application on shinyapps.io & use it", {
  confirm_existing_application_use("shinyapps.io")
})

@hadley - do you have different recommendations?

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 ----------------------------------------------------------
Expand Down