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

Restore test_dir(..., wrap = FALSE) #1183

Merged
merged 9 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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: 2 additions & 1 deletion R/parallel.R
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ test_files_parallel <- function(
env = NULL,
stop_on_failure = FALSE,
stop_on_warning = FALSE,
wrap = TRUE, # unused, to match test_files signature
load_package = c("none", "installed", "source")
) {

Expand Down Expand Up @@ -70,7 +71,7 @@ test_files_parallel <- function(
)

with_reporter(reporters$multi, {
parallel_update <- reporter$capabilities$parallel_update
parallel_update <- reporter$capabilities$parallel_updates
if (parallel_update) {
parallel_event_loop_smooth(queue, reporters)
} else {
Expand Down
39 changes: 32 additions & 7 deletions R/test-files.R
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,6 @@ test_dir <- function(path,
abort("No test files found")
}

if (!missing(wrap)) {
lifecycle::deprecate_warn("3.0.0", "test_dir(wrap = )")
}

want_parallel <- find_parallel(path, load_package, package)

if (is.null(reporter)) {
Expand All @@ -98,6 +94,22 @@ test_dir <- function(path,
reporter <- find_reporter(reporter)
parallel <- want_parallel && reporter$capabilities$parallel_support

if (parallel) {
if (!is_missing(wrap)) {
Copy link
Member

Choose a reason for hiding this comment

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

Given that you're the only person using this argument, I think you can just keep the warning the same for both serial and parallel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I did this. If for whatever reason someone ends up using wrap=FALSE in parallel mode they might be confused, but given they will get a deprecation warning hopefully they'll figure it out, and more likely such a person won't exist anyway.

lifecycle::deprecate_stop(
"3.0.0", "test_dir(wrap = )",
details=paste0(
"`wrap = ` may only be used with serial tests, but detected that ",
"parallel tests requested."
)
)
}
} else {
if (!is_missing(wrap)) {
lifecycle::deprecate_warn("3.0.0", "test_file(wrap = )")
}
}

test_files(
test_dir = path,
test_paths = test_paths,
Expand All @@ -107,6 +119,7 @@ test_dir <- function(path,
env = env,
stop_on_failure = stop_on_failure,
stop_on_warning = stop_on_warning,
wrap = wrap,
load_package = load_package,
parallel = parallel
)
Expand Down Expand Up @@ -150,10 +163,18 @@ test_files <- function(test_dir,
env = NULL,
stop_on_failure = FALSE,
stop_on_warning = FALSE,
wrap = lifecycle::deprecated(),
Copy link
Member

Choose a reason for hiding this comment

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

Just make this TRUE and then warn if wrap is FALSE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

load_package = c("none", "installed", "source"),
parallel = FALSE) {

if(is_missing(wrap)) {
wrap <- TRUE
}
if (parallel) {
if(!isTRUE(wrap)) {
warning('`wrap` must be TRUE in parallel mode.')
wrap <- TRUE
}
test_files <- test_files_parallel
} else {
test_files <- test_files_serial
Expand All @@ -168,6 +189,7 @@ test_files <- function(test_dir,
env = env,
stop_on_failure = stop_on_failure,
stop_on_warning = stop_on_warning,
wrap = wrap,
load_package = load_package
)
}
Expand All @@ -180,13 +202,16 @@ test_files_serial <- function(test_dir,
env = NULL,
stop_on_failure = FALSE,
stop_on_warning = FALSE,
wrap = TRUE,
load_package = c("none", "installed", "source")) {

env <- test_files_setup_env(test_package, test_dir, load_package, env)
test_files_setup_state(test_dir, test_package, load_helpers, env)
reporters <- test_files_reporter(reporter)

with_reporter(reporters$multi, lapply(test_paths, test_one_file, env = env))
with_reporter(reporters$multi,
lapply(test_paths, test_one_file, env = env, wrap = wrap)
)

test_files_check(reporters$list$get_results(),
stop_on_failure = stop_on_failure,
Expand Down Expand Up @@ -250,12 +275,12 @@ test_files_check <- function(results, stop_on_failure = TRUE, stop_on_warning =
invisible(results)
}

test_one_file <- function(path, env = test_env()) {
test_one_file <- function(path, env = test_env(), wrap = TRUE) {
reporter <- get_reporter()
on.exit(teardown_run(), add = TRUE)

reporter$start_file(path)
source_file(path, child_env(env))
source_file(path, child_env(env), wrap = wrap)
reporter$end_context_if_started()
reporter$end_file()
}
Expand Down
2 changes: 1 addition & 1 deletion R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ remove_source <- function(x) {
}
}

# Need to stip environment and source references to make lightweight
# Need to strip environment and source references to make lightweight
# function suitable to send to another process
transport_fun <- function(f) {
environment(f) <- .GlobalEnv
Expand Down