-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
moduleServer testing #2783
moduleServer testing #2783
Changes from 5 commits
946435f
14b572e
6fa332a
ed93d42
c2c0a0d
2c2ca4b
a036aa4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,23 +121,40 @@ createSessionProxy <- function(parentSession, ...) { | |
#' | ||
#' @export | ||
moduleServer <- function(id, module, session = getDefaultReactiveDomain()) { | ||
if (inherits(session, c("MockShinySession"))) session$isModuleServer <- TRUE | ||
callModule(module, id, session = session) | ||
} | ||
|
||
|
||
#' @rdname moduleServer | ||
#' @export | ||
callModule <- function(module, id, ..., session = getDefaultReactiveDomain()) { | ||
if (!inherits(session, "ShinySession") && !inherits(session, "session_proxy")) { | ||
stop("session must be a ShinySession or session_proxy object.") | ||
if (!inherits(session, c("ShinySession", "MockShinySession", "session_proxy"))) { | ||
stop("session must be a ShinySession, MockShinySession, or session_proxy object.") | ||
} | ||
childScope <- session$makeScope(id) | ||
|
||
withReactiveDomain(childScope, { | ||
if (!is.function(module)) { | ||
stop("module argument must be a function") | ||
} | ||
|
||
module(childScope$input, childScope$output, childScope, ...) | ||
}) | ||
if (inherits(session, "MockShinySession") && session$isModuleServer) { | ||
# If the module is under test *and* was called by moduleServer(), modify the | ||
# module function locally by inserting the equivalent of `session$env <- | ||
# environment()` at the beginning of its body. A similar operation is | ||
# performed by .testModule() if the module is *not* called through | ||
# moduleServer() but is under test. See .testModule() for details. | ||
body(module) <- rlang::expr({ | ||
base::assign("env", base::environment(), envir = !!session) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this line just be the same as the one down below? The other one is a more idiomatic way of doing it. |
||
!!!body(module) | ||
}) | ||
isolate( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this |
||
withReactiveDomain( | ||
session, | ||
withr::with_options(list(`shiny.allowoutputreads`=TRUE), { | ||
session$returned <- module(session$input, session$output, session, ...) | ||
}) | ||
) | ||
) | ||
} else { | ||
childScope <- session$makeScope(id) | ||
withReactiveDomain(childScope, { | ||
if (!is.function(module)) stop("module argument must be a function") | ||
module(childScope$input, childScope$output, childScope, ...) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,33 +64,55 @@ testModule <- function(module, expr, ...) { | |
) | ||
} | ||
|
||
isOldModule <- function(func) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be helpful to have a comment explaining the purpose of this function (and how it works). |
||
stopifnot(is.function(func)) | ||
required <- c("input", "output", "session") | ||
declared <- names(formals(func)) | ||
setequal(required, intersect(required, declared)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit more idiomatic to do something like: all(required %in% declared) |
||
} | ||
|
||
#' @noRd | ||
#' @importFrom withr with_options | ||
.testModule <- function(module, quosure, dots, env) { | ||
# Modify the module function locally by inserting `session$env <- | ||
# environment()` at the beginning of its body. The dynamic environment of the | ||
# module function is saved so that it may be referenced after the module | ||
# function has returned. The saved dynamic environment is the basis for the | ||
# `data` argument of tidy_eval() when used below to evaluate `quosure`, the | ||
# test code expression. | ||
body(module) <- rlang::expr({ | ||
session$env <- base::environment() | ||
!!!body(module) | ||
}) | ||
|
||
session <- MockShinySession$new() | ||
on.exit(if (!session$isClosed()) session$close()) | ||
args <- append(dots, list(input = session$input, output = session$output, session = session)) | ||
|
||
isolate( | ||
withReactiveDomain( | ||
session, | ||
withr::with_options(list(`shiny.allowoutputreads`=TRUE), { | ||
# Assigning to `$returned` causes a flush to happen automatically. | ||
session$returned <- do.call(module, args) | ||
}) | ||
if (isOldModule(module)) { | ||
# If the module is an "old-style" module that accepts input, output, and | ||
# session parameters, modify the function locally by inserting `session$env | ||
# <- environment()` at the beginning of its body. The dynamic environment of | ||
# the module function is saved so that it may be referenced after the module | ||
# function has returned. The saved dynamic environment is the basis for the | ||
# `data` argument of tidy_eval() when used below to evaluate `quosure`. | ||
body(module) <- rlang::expr({ | ||
session$env <- base::environment() | ||
!!!body(module) | ||
}) | ||
args <- append(dots, list(input = session$input, output = session$output, session = session)) | ||
isolate( | ||
withReactiveDomain( | ||
session, | ||
withr::with_options(list(`shiny.allowoutputreads`=TRUE), { | ||
# Assigning to `$returned` causes a flush to happen automatically. | ||
session$returned <- do.call(module, args) | ||
}) | ||
) | ||
) | ||
) | ||
} else { | ||
# If the module is a "new-style" module, we rely on logic in callModule() | ||
# that instruments the function if the session is a MockShinySession. | ||
# Appending additional arguments is not necessary, as input/output/session | ||
# will be provided in moduleServer(). `id` is also provided via | ||
# moduleServer(). | ||
isolate( | ||
withReactiveDomain( | ||
session, | ||
withr::with_options(list(`shiny.allowoutputreads`=TRUE), { | ||
# TODO Implement session$returned for new style modules | ||
do.call(module, dots) | ||
}) | ||
) | ||
) | ||
} | ||
|
||
# Evaluate `quosure` in a reactive context, and in the provided `env`, but | ||
# with `env` masked by a shallow view of `session$env`, the environment that | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
context("testModule-moduleServer") | ||
|
||
test_that("New-style modules work", { | ||
counterServer <- local({ | ||
function(id) { | ||
moduleServer(id, function(input, output, session) { | ||
count <- reactiveVal(0) | ||
observeEvent(input$button, { | ||
count(count() + 1) | ||
}) | ||
output$out <- renderText({ | ||
count() | ||
}) | ||
count | ||
}) | ||
} | ||
}) | ||
testModule(counterServer, { | ||
input$setInputs(button = 0) | ||
input$setInputs(button = 1) | ||
expect_equal(count(), 1) | ||
}, id = "foob") | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding
session$isModuleServer
here and checking it incallModule
, I think it would be clearer if the modifications tomodule
were done in this function. You could skip the call tocallModule
in the case wheresession
is aMockShinySession
.I also still feel a weird about embedding this test-related code into the
moduleServer
orcallModule
functions, but I don't see a better way to do it at the moment.