From c750df44483c76ac18b397ff27361aa7b38650b1 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 15:41:38 +0100 Subject: [PATCH 01/10] Fixes for local testing issues (#132) * Using the default plot device was causing `devtools::test()` to create a bunch of plot files in the test directory. I don't think there's any reason to use the default plot device (since that may lead to windows popping up for the user), so we just switch to using `pdf(NULL)` everywhere * There was one test that used `dev.new()` to start a new device; there I made sure that the new device was always a quiet `pdf()` device. * I also fixed a partial argument match warning that was causing a local test failure for me since I have `options(warnPartialMatchArgs = TRUE)` * Add a test for plotmath too (to ensure that (e.g.) we can compute string widths) --- R/eval.R | 7 +++---- tests/testthat/plot-persp.R | 2 +- tests/testthat/test-graphics.R | 6 +++--- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/R/eval.R b/R/eval.R index 8d2b84f7..0e599ce0 100644 --- a/R/eval.R +++ b/R/eval.R @@ -69,10 +69,9 @@ evaluate <- function(input, } if (new_device) { - # Start new graphics device and clean up afterwards - if (identical(grDevices::pdf, getOption("device"))) { - dev.new(file = NULL) - } else dev.new() + # Ensure we have a graphics device available for recording, but choose + # one that's available on all platforms and doesn't write to disk + pdf(file = NULL) dev.control(displaylist = "enable") dev <- dev.cur() on.exit(dev.off(dev)) diff --git a/tests/testthat/plot-persp.R b/tests/testthat/plot-persp.R index fd930539..47bfa2c6 100644 --- a/tests/testthat/plot-persp.R +++ b/tests/testthat/plot-persp.R @@ -1,4 +1,4 @@ -x <- seq(-10, 10, length = 30) +x <- seq(-10, 10, length.out = 30) y <- x ff <- function(x,y) { r <- sqrt(x^2 + y^2); 10 * sin(r) / r } z <- outer(x, y, ff) diff --git a/tests/testthat/test-graphics.R b/tests/testthat/test-graphics.R index 57687642..f13360fb 100644 --- a/tests/testthat/test-graphics.R +++ b/tests/testthat/test-graphics.R @@ -132,14 +132,14 @@ test_that("by default, evaluate() always records plots regardless of the device" }) test_that("Rplots.pdf files are not created", { - op <- options(device = pdf) - on.exit(options(op)) - evaluate(file("plot.R")) + ev <- evaluate("plot(1)") expect_false(file.exists("Rplots.pdf")) }) # https://github.com/yihui/knitr/issues/2297 test_that("existing plots will not leak into evaluate()", { + withr::local_options(device = function() pdf(NULL)) + pdf(NULL) dev.control('enable') d <- dev.cur() From 7d54732b477b07e78bdf32e041792bc97399f8b5 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 16:14:01 +0100 Subject: [PATCH 02/10] Clarify and simplify expr data type (#135) Make `parse_all()$expr` a list of expressions, where the length is the number of individual expressions within each top-level expression. --- R/eval.R | 46 +++++++++++++++---------------------- R/parse.R | 31 ++++++++++++++++++++----- man/parse_all.Rd | 26 ++++++++++++++++++--- tests/testthat/test-eval.R | 9 ++++++++ tests/testthat/test-parse.R | 9 ++++++++ 5 files changed, 85 insertions(+), 36 deletions(-) diff --git a/R/eval.R b/R/eval.R index 0e599ce0..af1703ae 100644 --- a/R/eval.R +++ b/R/eval.R @@ -95,12 +95,9 @@ evaluate <- function(input, if (length(dev.list()) < devn) dev.set(dev) devn <- length(dev.list()) - expr <- parsed$expr[[i]] - if (!is.null(expr)) - expr <- as.expression(expr) - out[[i]] <- evaluate_call( - expr, - parsed$src[[i]], + out[[i]] <- evaluate_top_level_expression( + exprs = parsed$expr[[i]], + src = parsed$src[[i]], envir = envir, enclos = enclos, debug = debug, @@ -128,27 +125,22 @@ evaluate <- function(input, unlist(out, recursive = FALSE, use.names = FALSE) } -evaluate_call <- function(call, - src = NULL, - envir = parent.frame(), - enclos = NULL, - debug = FALSE, - last = FALSE, - use_try = FALSE, - keep_warning = TRUE, - keep_message = TRUE, - log_echo = FALSE, - log_warning = FALSE, - output_handler = new_output_handler(), - include_timing = FALSE) { +evaluate_top_level_expression <- function(exprs, + src = NULL, + envir = parent.frame(), + enclos = NULL, + debug = FALSE, + last = FALSE, + use_try = FALSE, + keep_warning = TRUE, + keep_message = TRUE, + log_echo = FALSE, + log_warning = FALSE, + output_handler = new_output_handler(), + include_timing = FALSE) { + stopifnot(is.expression(exprs)) if (debug) message(src) - if (is.null(call) && !last) { - source <- new_source(src, call[[1]], output_handler$source) - return(list(source)) - } - stopifnot(is.call(call) || is.language(call) || is.atomic(call) || is.null(call)) - # Capture output w <- watchout(debug) on.exit(w$close()) @@ -161,7 +153,7 @@ evaluate_call <- function(call, cat(src, "\n", sep = "", file = stderr()) } - source <- new_source(src, call[[1]], output_handler$source) + source <- new_source(src, exprs[[1]], output_handler$source) output <- list(source) dev <- dev.cur() @@ -249,7 +241,7 @@ evaluate_call <- function(call, user_handlers <- output_handler$calling_handlers multi_args <- length(formals(value_handler)) > 1 - for (expr in call) { + for (expr in exprs) { srcindex <- length(output) time <- timing_fn(handle( ev <- withCallingHandlers( diff --git a/R/parse.R b/R/parse.R index b896a9df..f57cb868 100644 --- a/R/parse.R +++ b/R/parse.R @@ -7,11 +7,30 @@ #' If a connection, will be opened and closed only if it was closed initially. #' @param filename string overriding the file name #' @param allow_error whether to allow syntax errors in `x` -#' @return A data.frame with columns `src`, the source code, and -#' `expr`. If there are syntax errors in `x` and `allow_error = -#' TRUE`, the data frame has an attribute `PARSE_ERROR` that stores the -#' error object. +#' @return +#' A data frame with columns `src`, a character vector of source code, and +#' `expr`, a list-column of parsed expressions. There will be one row for each +#' top-level expression in `x`. A top-level expression is a complete expression +#' which would trigger execution if typed at the console. The `expression` +#' object in `expr` can be of any length: it will be 0 if the top-level +#' expression contains only whitespace and/or comments; 1 if the top-level +#' expression is a single scalar (like `TRUE`, `1`, or `"x"`), name, or call; +#' or 2 if the top-level expression uses `;` to put multiple expressions on +#' one line. +#' +#' If there are syntax errors in `x` and `allow_error = TRUE`, the data +#' frame will have an attribute `PARSE_ERROR` that stores the error object. #' @export +#' @examples +#' source <- " +#' # a comment +#' x +#' x;y +#' " +#' parsed <- parse_all(source) +#' lengths(parsed$expr) +#' str(parsed$expr) +#' parse_all <- function(x, filename = NULL, allow_error = FALSE) UseMethod("parse_all") #' @export @@ -40,7 +59,7 @@ parse_all.character <- function(x, filename = NULL, allow_error = FALSE) { # No code, only comments and/or empty lines ne <- length(exprs) if (ne == 0) { - return(data.frame(src = append_break(x), expr = I(rep(list(NULL), n)))) + return(data.frame(src = append_break(x), expr = I(rep(list(expression()), n)))) } srcref <- attr(exprs, "srcref", exact = TRUE) @@ -78,7 +97,7 @@ parse_all.character <- function(x, filename = NULL, allow_error = FALSE) { r <- p[1]:p[2] data.frame( src = x[r], - expr = I(rep(list(NULL), p[2] - p[1] + 1)), + expr = I(rep(list(expression()), p[2] - p[1] + 1)), line = r - 1 ) })) diff --git a/man/parse_all.Rd b/man/parse_all.Rd index 7b8a0f13..26eee692 100644 --- a/man/parse_all.Rd +++ b/man/parse_all.Rd @@ -15,11 +15,31 @@ If a connection, will be opened and closed only if it was closed initially.} \item{allow_error}{whether to allow syntax errors in \code{x}} } \value{ -A data.frame with columns \code{src}, the source code, and -\code{expr}. If there are syntax errors in \code{x} and \code{allow_error = TRUE}, the data frame has an attribute \code{PARSE_ERROR} that stores the -error object. +A data frame with columns \code{src}, a character vector of source code, and +\code{expr}, a list-column of parsed expressions. There will be one row for each +top-level expression in \code{x}. A top-level expression is a complete expression +which would trigger execution if typed at the console. The \code{expression} +object in \code{expr} can be of any length: it will be 0 if the top-level +expression contains only whitespace and/or comments; 1 if the top-level +expression is a single scalar (like \code{TRUE}, \code{1}, or \code{"x"}), name, or call; +or 2 if the top-level expression uses \verb{;} to put multiple expressions on +one line. + +If there are syntax errors in \code{x} and \code{allow_error = TRUE}, the data +frame will have an attribute \code{PARSE_ERROR} that stores the error object. } \description{ Works very similarly to parse, but also keeps original formatting and comments. } +\examples{ +source <- " + # a comment + x + x;y +" +parsed <- parse_all(source) +lengths(parsed$expr) +str(parsed$expr) + +} diff --git a/tests/testthat/test-eval.R b/tests/testthat/test-eval.R index ee7bef8f..1879d495 100644 --- a/tests/testthat/test-eval.R +++ b/tests/testthat/test-eval.R @@ -1,3 +1,12 @@ +test_that("can evaluate expressions of all lengths", { + source <- " + # a comment + 1 + x <- 2; x + " + expect_no_error(evaluate(source)) +}) + test_that("log_echo causes output to be immediately written to stderr()", { f <- function() { 1 diff --git a/tests/testthat/test-parse.R b/tests/testthat/test-parse.R index b66957c9..2c592125 100644 --- a/tests/testthat/test-parse.R +++ b/tests/testthat/test-parse.R @@ -1,3 +1,12 @@ +test_that("expr is always an expression", { + expect_equal(parse_all("#")$expr[[1]], expression()) + expect_equal(parse_all("1")$expr[[1]], expression(1), ignore_attr = "srcref") + expect_equal(parse_all("1;2")$expr[[1]], expression(1, 2), ignore_attr = "srcref") + + parsed <- parse_all("#\n1\n1;2") + expect_equal(lengths(parsed$expr), c(0, 1, 2)) +}) + test_that("{ not removed", { f <- function() { From 61b0c77d2668d089115b51d9403d87387c6a946c Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 16:14:53 +0100 Subject: [PATCH 03/10] Inline test files directly into tests and examples (#133) --- DESCRIPTION | 4 +- R/replay.R | 20 +++- man/replay.Rd | 20 +++- tests/testthat/_snaps/errors.md | 8 ++ tests/testthat/comment.R | 2 - tests/testthat/data.R | 2 - tests/testthat/error-complex.R | 5 - tests/testthat/error.R | 2 - tests/testthat/example-1.R | 22 ---- tests/testthat/ggplot-empty-1.R | 4 - tests/testthat/ggplot-empty-2.R | 4 - tests/testthat/ggplot-loop.R | 6 -- tests/testthat/ggplot.R | 2 - tests/testthat/helper.R | 8 ++ tests/testthat/interleave-1.R | 4 - tests/testthat/interleave-2.R | 4 - tests/testthat/order.R | 16 --- tests/testthat/parse.R | 6 -- tests/testthat/plot-additions.R | 2 - tests/testthat/plot-clip.R | 3 - tests/testthat/plot-last-comment.R | 4 - tests/testthat/plot-loop.R | 4 - tests/testthat/plot-multi-layout.R | 7 -- tests/testthat/plot-multi-layout2.R | 9 -- tests/testthat/plot-multi-missing.R | 4 - tests/testthat/plot-multi.R | 5 - tests/testthat/plot-new.R | 5 - tests/testthat/plot-par.R | 3 - tests/testthat/plot-par2.R | 5 - tests/testthat/plot-persp.R | 8 -- tests/testthat/plot-strwidth.R | 4 - tests/testthat/plot.R | 1 - tests/testthat/raw-output.R | 4 - tests/testthat/test-errors.R | 21 +++- tests/testthat/test-evaluate.R | 45 +++++++-- tests/testthat/test-graphics.R | 150 +++++++++++++++++++++------- tests/testthat/test-output.R | 2 +- tests/testthat/try.R | 4 - 38 files changed, 215 insertions(+), 214 deletions(-) create mode 100644 tests/testthat/_snaps/errors.md delete mode 100644 tests/testthat/comment.R delete mode 100644 tests/testthat/data.R delete mode 100644 tests/testthat/error-complex.R delete mode 100644 tests/testthat/error.R delete mode 100644 tests/testthat/example-1.R delete mode 100644 tests/testthat/ggplot-empty-1.R delete mode 100644 tests/testthat/ggplot-empty-2.R delete mode 100644 tests/testthat/ggplot-loop.R delete mode 100644 tests/testthat/ggplot.R create mode 100644 tests/testthat/helper.R delete mode 100644 tests/testthat/interleave-1.R delete mode 100644 tests/testthat/interleave-2.R delete mode 100644 tests/testthat/order.R delete mode 100644 tests/testthat/parse.R delete mode 100644 tests/testthat/plot-additions.R delete mode 100644 tests/testthat/plot-clip.R delete mode 100644 tests/testthat/plot-last-comment.R delete mode 100644 tests/testthat/plot-loop.R delete mode 100644 tests/testthat/plot-multi-layout.R delete mode 100644 tests/testthat/plot-multi-layout2.R delete mode 100644 tests/testthat/plot-multi-missing.R delete mode 100644 tests/testthat/plot-multi.R delete mode 100644 tests/testthat/plot-new.R delete mode 100644 tests/testthat/plot-par.R delete mode 100644 tests/testthat/plot-par2.R delete mode 100644 tests/testthat/plot-persp.R delete mode 100644 tests/testthat/plot-strwidth.R delete mode 100644 tests/testthat/plot.R delete mode 100644 tests/testthat/raw-output.R delete mode 100644 tests/testthat/try.R diff --git a/DESCRIPTION b/DESCRIPTION index a85039ce..fb36680a 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -28,7 +28,7 @@ Imports: methods Suggests: covr, - ggplot2, + ggplot2 (>= 3.3.6), lattice, rlang, testthat (>= 3.0.0), @@ -37,4 +37,4 @@ Config/Needs/website: tidyverse/tidytemplate Config/testthat/edition: 3 Encoding: UTF-8 Roxygen: list(markdown = TRUE) -RoxygenNote: 7.2.3 +RoxygenNote: 7.3.1 diff --git a/R/replay.R b/R/replay.R index d0b16c2e..3175a91c 100644 --- a/R/replay.R +++ b/R/replay.R @@ -6,12 +6,22 @@ #' @param x result from [evaluate()] #' @export #' @examples -#' samples <- system.file("tests", "testthat", package = "evaluate") -#' if (file_test("-d", samples)) { -#' replay(evaluate(file(file.path(samples, "order.R")))) -#' replay(evaluate(file(file.path(samples, "plot.R")))) -#' replay(evaluate(file(file.path(samples, "data.R")))) +#' f1 <- function() { +#' cat("1\n") +#' print("2") +#' warning("3") +#' print("4") +#' message("5") +#' stop("6") #' } +#' replay(evaluate("f1()")) +#' +#' f2 <- function() { +#' message("Hello") +#' plot(1:10) +#' message("Goodbye") +#' } +#' replay(evaluate("f2()")) replay <- function(x) UseMethod("replay", x) #' @export diff --git a/man/replay.Rd b/man/replay.Rd index b9f18b21..4ddbdf01 100644 --- a/man/replay.Rd +++ b/man/replay.Rd @@ -14,10 +14,20 @@ Replay a list of evaluated results, as if you'd run them in an R terminal. } \examples{ -samples <- system.file("tests", "testthat", package = "evaluate") -if (file_test("-d", samples)) { - replay(evaluate(file(file.path(samples, "order.R")))) - replay(evaluate(file(file.path(samples, "plot.R")))) - replay(evaluate(file(file.path(samples, "data.R")))) +f1 <- function() { + cat("1\n") + print("2") + warning("3") + print("4") + message("5") + stop("6") } +replay(evaluate("f1()")) + +f2 <- function() { + message("Hello") + plot(1:10) + message("Goodbye") +} +replay(evaluate("f2()")) } diff --git a/tests/testthat/_snaps/errors.md b/tests/testthat/_snaps/errors.md new file mode 100644 index 00000000..0a97eddb --- /dev/null +++ b/tests/testthat/_snaps/errors.md @@ -0,0 +1,8 @@ +# code errors if stop_on_error == 2L + + Code + evaluate("stop(\"1\")", stop_on_error = 2L) + Condition + Error: + ! 1 + diff --git a/tests/testthat/comment.R b/tests/testthat/comment.R deleted file mode 100644 index ca63e33f..00000000 --- a/tests/testthat/comment.R +++ /dev/null @@ -1,2 +0,0 @@ -# This test case contains no executable code -# but it shouldn't throw an error diff --git a/tests/testthat/data.R b/tests/testthat/data.R deleted file mode 100644 index fd772172..00000000 --- a/tests/testthat/data.R +++ /dev/null @@ -1,2 +0,0 @@ -data(barley, package = "lattice") -barley diff --git a/tests/testthat/error-complex.R b/tests/testthat/error-complex.R deleted file mode 100644 index 7df4d244..00000000 --- a/tests/testthat/error-complex.R +++ /dev/null @@ -1,5 +0,0 @@ -f <- function() g() -g <- function() h() -h <- function() stop("Error") - -f() diff --git a/tests/testthat/error.R b/tests/testthat/error.R deleted file mode 100644 index cf133e16..00000000 --- a/tests/testthat/error.R +++ /dev/null @@ -1,2 +0,0 @@ -stop("1") -2 diff --git a/tests/testthat/example-1.R b/tests/testthat/example-1.R deleted file mode 100644 index a2a58e0a..00000000 --- a/tests/testthat/example-1.R +++ /dev/null @@ -1,22 +0,0 @@ -# These test cases check that interweave -# works for a variety of situations - -a <- 1 # Comment after an expression -b <- 2 - -{ - a - b -} - -# Here is a comment which should be followed -# by two new lines - -{ - print(a) # comment in a block - print(b) -} - -a; b - -a; b # Comment diff --git a/tests/testthat/ggplot-empty-1.R b/tests/testthat/ggplot-empty-1.R deleted file mode 100644 index 704d961e..00000000 --- a/tests/testthat/ggplot-empty-1.R +++ /dev/null @@ -1,4 +0,0 @@ -suppressPackageStartupMessages(library(ggplot2)) -ggplot(iris) + - aes(x = Speciess, y = Sepal.Length) + - geom_boxplot() diff --git a/tests/testthat/ggplot-empty-2.R b/tests/testthat/ggplot-empty-2.R deleted file mode 100644 index 15745561..00000000 --- a/tests/testthat/ggplot-empty-2.R +++ /dev/null @@ -1,4 +0,0 @@ -suppressPackageStartupMessages(library(ggplot2)) -ggplot(iris) + - aes(x = Species, y = Sepal.Length) + - geom_bar() diff --git a/tests/testthat/ggplot-loop.R b/tests/testthat/ggplot-loop.R deleted file mode 100644 index 59d8a122..00000000 --- a/tests/testthat/ggplot-loop.R +++ /dev/null @@ -1,6 +0,0 @@ -suppressPackageStartupMessages(library(ggplot2)) -for (j in 1:2) { - # ggplot2 has been loaded previously - print(ggplot(data.frame(x = rnorm(30), y = runif(30)), aes(x, y)) + geom_point()) -} - diff --git a/tests/testthat/ggplot.R b/tests/testthat/ggplot.R deleted file mode 100644 index e631ab2f..00000000 --- a/tests/testthat/ggplot.R +++ /dev/null @@ -1,2 +0,0 @@ -suppressPackageStartupMessages(library(ggplot2)) -ggplot(mtcars, aes(mpg, wt)) + geom_point() diff --git a/tests/testthat/helper.R b/tests/testthat/helper.R new file mode 100644 index 00000000..4091f6a6 --- /dev/null +++ b/tests/testthat/helper.R @@ -0,0 +1,8 @@ +evaluate_ <- function(text, ...) { + # Trim off leading/trailing new lines and dedent + text <- gsub("^\n {4}", "", text) + text <- gsub("\n {4}", "\n", text) + text <- gsub("\n +$", "", text) + + evaluate(text, ...) +} diff --git a/tests/testthat/interleave-1.R b/tests/testthat/interleave-1.R deleted file mode 100644 index 59042732..00000000 --- a/tests/testthat/interleave-1.R +++ /dev/null @@ -1,4 +0,0 @@ -for (i in 1:2) { - cat(i) - plot(i) -} diff --git a/tests/testthat/interleave-2.R b/tests/testthat/interleave-2.R deleted file mode 100644 index af03d330..00000000 --- a/tests/testthat/interleave-2.R +++ /dev/null @@ -1,4 +0,0 @@ -for (i in 1:2) { - plot(i) - cat(i) -} diff --git a/tests/testthat/order.R b/tests/testthat/order.R deleted file mode 100644 index 852dc602..00000000 --- a/tests/testthat/order.R +++ /dev/null @@ -1,16 +0,0 @@ -cat("1\n") -print("2") -warning("3") -print("4") -message("5") -stop("6") -stop("7", call. = FALSE) - -f <- function(x) { - print("8") - message("9") - warning("10") - stop("11") -} -f() - diff --git a/tests/testthat/parse.R b/tests/testthat/parse.R deleted file mode 100644 index efc95a5a..00000000 --- a/tests/testthat/parse.R +++ /dev/null @@ -1,6 +0,0 @@ -f <- function() { - for (i in 1:3) { - plot(rnorm(100)) - lines(rnorm(100)) - } -} diff --git a/tests/testthat/plot-additions.R b/tests/testthat/plot-additions.R deleted file mode 100644 index 253b6f9b..00000000 --- a/tests/testthat/plot-additions.R +++ /dev/null @@ -1,2 +0,0 @@ -plot(1:10) -lines(1:10) diff --git a/tests/testthat/plot-clip.R b/tests/testthat/plot-clip.R deleted file mode 100644 index 1246cef6..00000000 --- a/tests/testthat/plot-clip.R +++ /dev/null @@ -1,3 +0,0 @@ -plot(rnorm(100), rnorm(100)) -clip(-1, 1, -1, 1) -points(rnorm(100), rnorm(100), col = 'red') diff --git a/tests/testthat/plot-last-comment.R b/tests/testthat/plot-last-comment.R deleted file mode 100644 index 2bbd4354..00000000 --- a/tests/testthat/plot-last-comment.R +++ /dev/null @@ -1,4 +0,0 @@ -par(mfrow = c(3, 3)) -for (i in 1:7) - image(volcano) -# comment diff --git a/tests/testthat/plot-loop.R b/tests/testthat/plot-loop.R deleted file mode 100644 index 10342e06..00000000 --- a/tests/testthat/plot-loop.R +++ /dev/null @@ -1,4 +0,0 @@ -for (i in 1:3) { - plot(rnorm(100)) -} - diff --git a/tests/testthat/plot-multi-layout.R b/tests/testthat/plot-multi-layout.R deleted file mode 100644 index 41fb3d57..00000000 --- a/tests/testthat/plot-multi-layout.R +++ /dev/null @@ -1,7 +0,0 @@ -for (j in 1:3) { - layout(matrix(c(1, 2, 1, 3, 4, 4), 3, 2, byrow = TRUE)) - plot(rnorm(10)) - plot(rnorm(10)) - plot(rnorm(10)) - plot(rnorm(10)) -} diff --git a/tests/testthat/plot-multi-layout2.R b/tests/testthat/plot-multi-layout2.R deleted file mode 100644 index 20d12805..00000000 --- a/tests/testthat/plot-multi-layout2.R +++ /dev/null @@ -1,9 +0,0 @@ -layout(matrix(c(1, 2, 1, 3, 4, 4), 3, 2, byrow = TRUE)) -# another expression before drawing the plots -x <- 1 + 1 -for (j in 1:2) { - plot(rnorm(10)) - plot(rnorm(10)) - plot(rnorm(10)) - plot(rnorm(10)) -} diff --git a/tests/testthat/plot-multi-missing.R b/tests/testthat/plot-multi-missing.R deleted file mode 100644 index 402a9747..00000000 --- a/tests/testthat/plot-multi-missing.R +++ /dev/null @@ -1,4 +0,0 @@ -par(mfrow = c(2, 2)) -plot(1) -plot(2) -plot(3) diff --git a/tests/testthat/plot-multi.R b/tests/testthat/plot-multi.R deleted file mode 100644 index 6ab65579..00000000 --- a/tests/testthat/plot-multi.R +++ /dev/null @@ -1,5 +0,0 @@ -par(mfrow = c(2, 2)) -plot(1) -plot(2) -plot(3) -plot(4) diff --git a/tests/testthat/plot-new.R b/tests/testthat/plot-new.R deleted file mode 100644 index 73782602..00000000 --- a/tests/testthat/plot-new.R +++ /dev/null @@ -1,5 +0,0 @@ -plot.new() -plot(1:10) -plot.new() -plot(1:10) -plot.new() diff --git a/tests/testthat/plot-par.R b/tests/testthat/plot-par.R deleted file mode 100644 index ada66433..00000000 --- a/tests/testthat/plot-par.R +++ /dev/null @@ -1,3 +0,0 @@ -plot(1) -par(mar = rep(0, 4)) -plot(2) diff --git a/tests/testthat/plot-par2.R b/tests/testthat/plot-par2.R deleted file mode 100644 index e56c85c6..00000000 --- a/tests/testthat/plot-par2.R +++ /dev/null @@ -1,5 +0,0 @@ -barplot(table(mtcars$mpg), main = "All") -# should capture all plots in this loop -for (numcyl in levels(as.factor(mtcars$cyl))) { - barplot(table(mtcars$mpg[mtcars$cyl == numcyl]), main = paste("cyl = ", numcyl)) -} diff --git a/tests/testthat/plot-persp.R b/tests/testthat/plot-persp.R deleted file mode 100644 index 47bfa2c6..00000000 --- a/tests/testthat/plot-persp.R +++ /dev/null @@ -1,8 +0,0 @@ -x <- seq(-10, 10, length.out = 30) -y <- x -ff <- function(x,y) { r <- sqrt(x^2 + y^2); 10 * sin(r) / r } -z <- outer(x, y, ff) -z[is.na(z)] <- 1 -for (i in 1:3) { - persp(x, y, z, phi = 30 + i * 10, theta = 30) -} diff --git a/tests/testthat/plot-strwidth.R b/tests/testthat/plot-strwidth.R deleted file mode 100644 index 3739b982..00000000 --- a/tests/testthat/plot-strwidth.R +++ /dev/null @@ -1,4 +0,0 @@ -x <- strwidth('foo', 'inches') -y <- strheight('foo', 'inches') -par(mar = c(4, 4, 1, 1)) -plot(1) diff --git a/tests/testthat/plot.R b/tests/testthat/plot.R deleted file mode 100644 index e6e140b9..00000000 --- a/tests/testthat/plot.R +++ /dev/null @@ -1 +0,0 @@ -plot(1:10) diff --git a/tests/testthat/raw-output.R b/tests/testthat/raw-output.R deleted file mode 100644 index 1f445125..00000000 --- a/tests/testthat/raw-output.R +++ /dev/null @@ -1,4 +0,0 @@ -rnorm(10) -x <- list("I'm a list!") -suppressPackageStartupMessages(library(ggplot2)) -ggplot(mtcars, aes(mpg, wt)) + geom_point() diff --git a/tests/testthat/test-errors.R b/tests/testthat/test-errors.R index 274f439d..163044ea 100644 --- a/tests/testthat/test-errors.R +++ b/tests/testthat/test-errors.R @@ -1,19 +1,25 @@ test_that("all code run, even after error", { - ev <- evaluate(file("error.R")) + ev <- evaluate_('stop("1")\n2') expect_length(ev, 4) }) test_that("code aborts on error if stop_on_error == 1L", { - ev <- evaluate(file("error.R"), stop_on_error = 1L) + ev <- evaluate('stop("1")\n2', stop_on_error = 1L) expect_length(ev, 2) }) test_that("code errors if stop_on_error == 2L", { - expect_error(evaluate(file("error.R"), stop_on_error = 2L), "1") + expect_snapshot(evaluate('stop("1")', stop_on_error = 2L), error = TRUE) }) test_that("traceback useful if stop_on_error == 2L", { - expect_error(evaluate(file("error-complex.R"), stop_on_error = 2L), "Error") + expect_error(evaluate_(' + f <- function() g() + g <- function() h() + h <- function() stop("Error") + + f() + ', stop_on_error = 2L), "Error") ## Doesn't work because .Traceback not create when code run ## inside try or tryCatch. Can't figure out how to work around. @@ -26,6 +32,11 @@ test_that("traceback useful if stop_on_error == 2L", { }) test_that("capture messages in try() (#88)", { - ev <- evaluate(file("try.R")) + ev <- evaluate_(' + g <- function() f("error") + f <- function(x) stop(paste0("Obscure ", x)) + + try(g()) + ') expect_match(ev[[length(ev)]], "Obscure error") }) diff --git a/tests/testthat/test-evaluate.R b/tests/testthat/test-evaluate.R index e607d74d..8d186e19 100644 --- a/tests/testthat/test-evaluate.R +++ b/tests/testthat/test-evaluate.R @@ -1,5 +1,8 @@ test_that("file with only comments runs", { - ev <- evaluate(file("comment.R")) + ev <- evaluate_(" + # This test case contains no executable code + # but it shouldn't throw an error + ") expect_length(ev, 2) expect_equal(classes(ev), c("source", "source")) @@ -8,7 +11,10 @@ test_that("file with only comments runs", { test_that("data sets loaded", { skip_if_not_installed("lattice") - ev <- evaluate(file("data.R")) + ev <- evaluate_(' + data(barley, package = "lattice") + barley + ') expect_length(ev, 3) }) @@ -62,21 +68,38 @@ test_that("options(warn = 0) and options(warn = 1) produces warnings", { # }) test_that("output and plots interleaved correctly", { - ev <- evaluate(file("interleave-1.R")) - expect_equal(classes(ev), - c("source", "character", "recordedplot", "character", "recordedplot")) + ev <- evaluate_(" + for (i in 1:2) { + cat(i) + plot(i) + } + ") + expect_equal( + classes(ev), + c("source", "character", "recordedplot", "character", "recordedplot") + ) - ev <- evaluate(file("interleave-2.R")) - expect_equal(classes(ev), - c("source", "recordedplot", "character", "recordedplot", "character")) + ev <- evaluate_(" + for (i in 1:2) { + plot(i) + cat(i) + } + ") + expect_equal( + classes(ev), + c("source", "recordedplot", "character", "recordedplot", "character") + ) }) test_that("return value of value handler inserted directly in output list", { skip_if_not_installed("ggplot2") - ev <- evaluate( - file("raw-output.R"), - output_handler = new_output_handler(value = identity) + ev <- evaluate_(' + rnorm(10) + x <- list("I\'m a list!") + suppressPackageStartupMessages(library(ggplot2)) + ggplot(mtcars, aes(mpg, wt)) + geom_point() + ', output_handler = new_output_handler(value = identity) ) expect_equal( classes(ev), diff --git a/tests/testthat/test-graphics.R b/tests/testthat/test-graphics.R index f13360fb..014f4eb0 100644 --- a/tests/testthat/test-graphics.R +++ b/tests/testthat/test-graphics.R @@ -1,19 +1,22 @@ test_that("single plot is captured", { - ev <- evaluate(file("plot.R")) + ev <- evaluate("plot(1:10)") expect_length(ev, 2) expect_equal(classes(ev), c("source", "recordedplot")) }) test_that("ggplot is captured", { skip_if_not_installed("ggplot2") - - ev <- evaluate(file("ggplot.R")) - expect_length(ev, 3) - expect_equal(classes(ev), c("source", "source", "recordedplot")) + ev <- evaluate( + "ggplot2::ggplot(mtcars, ggplot2::aes(mpg, wt)) + ggplot2::geom_point()" + ) + expect_equal(classes(ev), c("source", "recordedplot")) }) test_that("plot additions are captured", { - ev <- evaluate(file("plot-additions.R")) + ev <- evaluate_(" + plot(1:10) + lines(1:10) + ") expect_length(ev, 4) expect_equal( @@ -23,64 +26,114 @@ test_that("plot additions are captured", { }) test_that("blank plots by plot.new() are preserved", { - ev <- evaluate(file("plot-new.R")) + ev <- evaluate_(" + plot.new() + plot(1:10) + plot.new() + plot(1:10) + plot.new() + ") expect_length(ev, 10) - - expect_equal( - classes(ev), - rep(c("source", "recordedplot"), 5) - ) + expect_equal(classes(ev), rep(c("source", "recordedplot"), 5)) }) test_that("base plots in a single expression are captured", { - ev <- evaluate(file("plot-loop.R")) + ev <- evaluate_(" + for (i in 1:3) { + plot(rnorm(100)) + } + ") expect_length(ev, 4) - expect_equal(classes(ev), c("source", rep("recordedplot", 3))) }) test_that("ggplot2 plots in a single expression are captured", { skip_if_not_installed("ggplot2") - ev <- evaluate(file("ggplot-loop.R")) + ev <- evaluate_(" + suppressPackageStartupMessages(library(ggplot2)) + for (j in 1:2) { + # ggplot2 has been loaded previously + print(ggplot(data.frame(x = rnorm(30), y = runif(30)), aes(x, y)) + geom_point()) + } + ") expect_length(ev, 4) expect_equal(classes(ev), c(rep("source", 2), rep("recordedplot", 2))) }) -test_that("Empty ggplot should not be recorded", { +test_that("erroring ggplots should not be recorded", { skip_if_not_installed("ggplot2") - ev <- evaluate(file(test_path("ggplot-empty-1.R"))) - expect_identical(classes(ev), c( - "source", "source", - if (packageVersion("ggplot2") > "3.3.6") "rlang_error" else "simpleError" - )) - ev <- evaluate(file(test_path("ggplot-empty-2.R"))) + + # error in aesthetics + ev <- evaluate_(" + suppressPackageStartupMessages(library(ggplot2)) + ggplot(iris) + aes(XXXXXXXXXX, Sepal.Length) + geom_boxplot() + ") + expect_identical(classes(ev), c("source", "source", "rlang_error")) + + # error in geom + ev <- evaluate_(" + suppressPackageStartupMessages(library(ggplot2)) + ggplot(iris) + aes(Species, Sepal.Length) + geom_bar() + ") expect_identical(classes(ev), c("source", "source", "rlang_error")) }) test_that("multirow graphics are captured only when complete", { - ev <- evaluate(file("plot-multi.R")) - + ev <- evaluate_(" + par(mfrow = c(2, 2)) + plot(1) + plot(2) + plot(3) + plot(4) + ") expect_equal(classes(ev), c(rep("source", 5), "recordedplot")) }) test_that("multirow graphics are captured on close", { - ev <- evaluate(file("plot-multi-missing.R")) + ev <- evaluate_(" + par(mfrow = c(2, 2)) + plot(1) + plot(2) + plot(3) + ") expect_equal(classes(ev), c(rep("source", 4), "recordedplot")) }) test_that("plots are captured in a non-rectangular layout", { - ev <- evaluate(file("plot-multi-layout.R")) + ev <- evaluate_(" + for (j in 1:3) { + layout(matrix(c(1, 2, 1, 3, 4, 4), 3, 2, byrow = TRUE)) + plot(rnorm(10)) + plot(rnorm(10)) + plot(rnorm(10)) + plot(rnorm(10)) + } + ") expect_equal(classes(ev), rep(c("source", "recordedplot"), c(1, 3))) - ev <- evaluate(file("plot-multi-layout2.R")) + ev <- evaluate_(" + layout(matrix(c(1, 2, 1, 3, 4, 4), 3, 2, byrow = TRUE)) + # another expression before drawing the plots + x <- 1 + 1 + for (j in 1:2) { + plot(rnorm(10)) + plot(rnorm(10)) + plot(rnorm(10)) + plot(rnorm(10)) + } + ") expect_equal(classes(ev), rep(c("source", "recordedplot"), c(4, 2))) }) test_that("changes in parameters don't generate new plots", { - ev <- evaluate(file("plot-par.R")) + ev <- evaluate_(" + plot(1) + par(mar = rep(0, 4)) + plot(2) + ") expect_equal( classes(ev), c("source", "recordedplot", "source", "source", "recordedplot") @@ -88,27 +141,56 @@ test_that("changes in parameters don't generate new plots", { }) test_that("plots in a loop are captured even the changes seem to be from par only", { - ev <- evaluate(file("plot-par2.R")) + ev <- evaluate_(' + barplot(table(mtcars$mpg), main = "All") + # should capture all plots in this loop + for (numcyl in levels(as.factor(mtcars$cyl))) { + barplot(table(mtcars$mpg[mtcars$cyl == numcyl]), main = paste("cyl = ", numcyl)) + } + ') expect_equal(classes(ev), c("source", "recordedplot")[c(1, 2, 1, 1, 2, 2, 2)]) }) test_that("strwidth()/strheight() should not produce new plots", { - ev <- evaluate(file("plot-strwidth.R")) + ev <- evaluate_(" + x <- strwidth('foo', 'inches') + y <- strheight('foo', 'inches') + par(mar = c(4, 4, 1, 1)) + plot(1) + ") expect_equal(classes(ev), rep(c("source", "recordedplot"), c(4, 1))) }) test_that("clip() does not produce new plots", { - ev <- evaluate(file("plot-clip.R")) + ev <- evaluate_(" + plot(rnorm(100), rnorm(100)) + clip(-1, 1, -1, 1) + points(rnorm(100), rnorm(100), col = 'red') + ") expect_equal(classes(ev), c("source", "recordedplot")[c(1, 2, 1, 1, 2)]) }) test_that("perspective plots are captured", { - ev <- evaluate(file("plot-persp.R")) + ev <- evaluate_(" + x <- seq(-10, 10, length.out = 30) + y <- x + ff <- function(x,y) { r <- sqrt(x^2 + y^2); 10 * sin(r) / r } + z <- outer(x, y, ff) + z[is.na(z)] <- 1 + for (i in 1:3) { + persp(x, y, z, phi = 30 + i * 10, theta = 30) + } + ") expect_equal(classes(ev), rep(c("source", "recordedplot"), c(6, 3))) }) test_that("an incomplete plot with a comment in the end is also captured", { - ev <- evaluate(file("plot-last-comment.R")) + ev <- evaluate_(" + par(mfrow = c(3, 3)) + for (i in 1:7) + image(volcano) + # comment + ") expect_equal(classes(ev), rep(c("source", "recordedplot"), c(3, 1))) }) @@ -120,7 +202,7 @@ test_that("repeatedly drawing the same plot does not omit plots randomly", { # test_that("no plot windows open", { # graphics.off() # expect_equal(length(dev.list()), 0) -# evaluate(file("plot.R")) +# evaluate("plot(1)") # expect_equal(length(dev.list()), 0) # }) diff --git a/tests/testthat/test-output.R b/tests/testthat/test-output.R index a7bc251b..ffe52daa 100644 --- a/tests/testthat/test-output.R +++ b/tests/testthat/test-output.R @@ -1,6 +1,6 @@ test_that("open plot windows maintained", { n <- length(dev.list()) - evaluate(file("plot.R")) + evaluate("plot(1)") expect_length(dev.list(), n) }) diff --git a/tests/testthat/try.R b/tests/testthat/try.R deleted file mode 100644 index 5ae5daa8..00000000 --- a/tests/testthat/try.R +++ /dev/null @@ -1,4 +0,0 @@ -g <- function() f("error") -f <- function(x) stop(paste0("Obscure ", x)) - -try(g()) From e1ece68d9b2211e16301ae233f3af1a8b6846472 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 17:07:08 +0100 Subject: [PATCH 04/10] Create one test for all warning options (#134) * Set options using withr * Also test `warn = 2` since testthat bug has long been fixed --- tests/testthat/test-evaluate.R | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/tests/testthat/test-evaluate.R b/tests/testthat/test-evaluate.R index 8d186e19..857c297b 100644 --- a/tests/testthat/test-evaluate.R +++ b/tests/testthat/test-evaluate.R @@ -48,24 +48,27 @@ test_that("errors during printing visible values are captured", { expect_s3_class(ev[[2]], "error") }) -test_that("options(warn = -1) suppresses warnings", { - ev <- evaluate("op = options(warn = -1); warning('hi'); options(op)") +test_that("respects warn options", { + # suppress warnings + withr::local_options(warn = -1) + ev <- evaluate("warning('hi')") expect_equal(classes(ev), "source") -}) -test_that("options(warn = 0) and options(warn = 1) produces warnings", { - ev <- evaluate("op = options(warn = 0); warning('hi'); options(op)") + # delayed warnings are always immediate in knitr + withr::local_options(warn = 0) + ev <- evaluate("warning('hi')") expect_equal(classes(ev), c("source", "simpleWarning")) - ev <- evaluate("op = options(warn = 1); warning('hi'); options(op)") + # immediate warnings + withr::local_options(warn = 1) + ev <- evaluate("warning('hi')") expect_equal(classes(ev), c("source", "simpleWarning")) -}) -# See https://github.com/r-lib/evaluate/pull/81#issuecomment-367685196 -# test_that("options(warn = 2) produces errors instead of warnings", { -# ev_warn_2 <- evaluate("op = options(warn = 2); warning('hi'); options(op)") -# expect_equal(classes(ev_warn_2), c("source", "simpleError")) -# }) + # warnings become errors + withr::local_options(warn = 2) + ev <- evaluate("warning('hi')") + expect_equal(classes(ev), c("source", "simpleError")) +}) test_that("output and plots interleaved correctly", { ev <- evaluate_(" From ec159c035aee39783e8e652b3023434baecf56b6 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 17:08:02 +0100 Subject: [PATCH 05/10] Refactor plot_snapshot (#138) The logic should be identical, but hopefully it's now a bit easier to understand what's going on. --- R/graphics.R | 105 +++++++++++++++++++++++++++++---------------------- 1 file changed, 59 insertions(+), 46 deletions(-) diff --git a/R/graphics.R b/R/graphics.R index 0a054833..a1a87347 100644 --- a/R/graphics.R +++ b/R/graphics.R @@ -10,65 +10,78 @@ plot_snapshot <- local({ last_plot <- NULL function(incomplete = FALSE) { - # to record a plot, at least one device must be open; the list of devices - # must not have changed since evaluate() started - if (is.null(devs <- dev.list()) || !identical(devs, .env$dev_list)) return(NULL) - if (!incomplete && !par('page')) return(NULL) # current page not complete + devs <- dev.list() + # No graphics devices + if (is.null(devs)) { + return() + } + + # Current graphics device changed since evaluate started + if (!identical(devs, .env$dev_list)) { + return() + } + + # current page is incomplete + if (!par("page") && !incomplete) { + return() + } plot <- recordPlot() - if (identical(last_plot, plot) || is_par_change(last_plot, plot)) { - return(NULL) + if (!makes_visual_change(plot[[1]])) { + return() + } + + if (!looks_different(last_plot[[1]], plot[[1]])) { + return() } - if (is.empty(plot)) return(NULL) last_plot <<- plot plot } }) -is_par_change <- function(p1, p2) { - calls1 <- plot_calls(p1) - calls2 <- plot_calls(p2) - - n1 <- length(calls1) - n2 <- length(calls2) +looks_different <- function(old_dl, new_dl) { + if (identical(old_dl, new_dl)) { + return(FALSE) + } - if (n2 <= n1) return(FALSE) - i1 <- seq_len(n1) - if (!identical(calls1, calls2[i1])) return(FALSE) - # also check if the content of the display list is still the same (note we - # need p1[[1]][] as well because [] turns a dotted pair list into a list) - if (!identical(p1[[1]][i1], p2[[1]][i1])) return(FALSE) + # If the new plot has fewer calls, it must be a visual change + if (length(new_dl) < length(old_dl)) { + return(TRUE) + } + + # If the initial calls are different, it must be a visual change + if (!identical(old_dl[], new_dl[seq_along(old_dl)])) { + return(TRUE) + } - last <- calls2[(n1 + 1):n2] - all(last %in% empty_calls) + # If the last calls involve visual changes then it's a visual change + added_dl <- new_dl[-seq_along(old_dl)] + makes_visual_change(added_dl) } -# if all calls are in these elements, the plot is basically empty -empty_calls <- c("layout", "par", "clip") -empty_calls <- c( - "palette", "palette2", - sprintf("C_%s", c(empty_calls, "strWidth", "strHeight", "plot_window")) -) - -is.empty <- function(x) { - if (is.null(x)) return(TRUE) - - pc <- plot_calls(x) - if (length(pc) == 0) return(TRUE) +makes_visual_change <- function(plot) { + xs <- lapply(plot, function(x) x[[2]][[1]]) - all(pc %in% empty_calls) + for (x in xs) { + if (hasName(x, "name")) { # base graphics + if (!x$name %in% non_visual_calls) { + return(TRUE) + } + } else if (is.call(x)) { # grid graphics + if (as.character(x[[1]]) != "requireNamespace") { + return(TRUE) + } + } + } + FALSE } -plot_calls <- function(plot) { - el <- lapply(plot[[1]], "[[", 2) - if (length(el) == 0) return() - unlist(lapply(el, function(x) { - # grid graphics do not have x[[1]]$name - if (!is.null(nm <- x[[1]][["name"]])) return(nm) - nm <- deparse(x[[1]]) - # the plot element should not be empty, and ignore calls that are simply - # requireNamespace() - if (length(x[[2]]) > 0 || !all(grepl("^requireNamespace\\(", nm))) nm - })) -} +non_visual_calls <- c( + "C_clip", + "C_layout", + "C_par", + "C_plot_window", + "C_strHeight", "C_strWidth", + "palette", "palette2" +) From 7f8d7d567200dbca472d28d1d264333ec576c625 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 17:08:49 +0100 Subject: [PATCH 06/10] Move flush_console() to its own file (#141) * Make it more self-contained * Add a test * Polish docs --- R/eval.R | 5 +---- R/flush-console.R | 35 +++++++++++++++++++++++++++++ R/utils.R | 4 ++++ R/watcher.R | 17 -------------- man/flush_console.Rd | 7 +++--- tests/testthat/test-flush-console.R | 27 ++++++++++++++++++++++ 6 files changed, 70 insertions(+), 25 deletions(-) create mode 100644 R/flush-console.R create mode 100644 R/utils.R create mode 100644 tests/testthat/test-flush-console.R diff --git a/R/eval.R b/R/eval.R index af1703ae..fe994ea3 100644 --- a/R/eval.R +++ b/R/eval.R @@ -165,10 +165,7 @@ evaluate_top_level_expression <- function(exprs, output <<- c(output, out) } - flush_old <- .env$flush_console; on.exit({ - .env$flush_console <- flush_old - }, add = TRUE) - .env$flush_console <- function() handle_output(FALSE) + local_output_handler(function() handle_output(FALSE)) # Hooks to capture plot creation capture_plot <- function() { diff --git a/R/flush-console.R b/R/flush-console.R new file mode 100644 index 00000000..22290df3 --- /dev/null +++ b/R/flush-console.R @@ -0,0 +1,35 @@ +#' An emulation of `flush.console()` in `evaluate()` +#' +#' @description +#' When [evaluate()] is evaluating code, the text output is diverted into +#' an internal connection, and there is no way to flush that connection. This +#' function provides a way to "flush" the connection so that any text output can +#' be immediately written out, and more importantly, the `text` handler +#' (specified in the `output_handler` argument of `evaluate()`) will +#' be called, which makes it possible for users to know it when the code +#' produces text output using the handler. +#' +#' This function is supposed to be called inside `evaluate()` (e.g. +#' either a direct `evaluate()` call or in \pkg{knitr} code chunks). +#' @export +flush_console = function() { + if (!is.null(.env$output_handler)) { + .env$output_handler() + } + invisible() +} + +.env = new.env() +.env$output_handler <- NULL + +set_output_handler <- function(handler) { + old <- .env$output_handler + .env$output_handler <- handler + invisible(old) +} + +local_output_handler <- function(handler, frame = parent.frame()) { + old <- set_output_handler(handler) + defer(set_output_handler(old), frame) + invisible() +} diff --git a/R/utils.R b/R/utils.R new file mode 100644 index 00000000..c46dda23 --- /dev/null +++ b/R/utils.R @@ -0,0 +1,4 @@ +defer <- function(expr, frame = parent.frame(), after = FALSE) { + thunk <- as.call(list(function() expr)) + do.call(on.exit, list(thunk, TRUE, after), envir = frame) +} diff --git a/R/watcher.R b/R/watcher.R index 1864f62f..cb37379d 100644 --- a/R/watcher.R +++ b/R/watcher.R @@ -58,20 +58,3 @@ test_con = function(con, test) { con_error = function(x) stop( x, '... Please make sure not to call closeAllConnections().', call. = FALSE ) - -.env = new.env() -.env$flush_console = function() {} - -#' An emulation of flush.console() in evaluate() -#' -#' When [evaluate()] is evaluating code, the text output is diverted into -#' an internal connection, and there is no way to flush that connection. This -#' function provides a way to "flush" the connection so that any text output can -#' be immediately written out, and more importantly, the `text` handler -#' (specified in the `output_handler` argument of `evaluate()`) will -#' be called, which makes it possible for users to know it when the code -#' produces text output using the handler. -#' @note This function is supposed to be called inside `evaluate()` (e.g. -#' either a direct `evaluate()` call or in \pkg{knitr} code chunks). -#' @export -flush_console = function() .env$flush_console() diff --git a/man/flush_console.Rd b/man/flush_console.Rd index 85e362d7..7ae3176f 100644 --- a/man/flush_console.Rd +++ b/man/flush_console.Rd @@ -1,8 +1,8 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/watcher.R +% Please edit documentation in R/flush-console.R \name{flush_console} \alias{flush_console} -\title{An emulation of flush.console() in evaluate()} +\title{An emulation of \code{flush.console()} in \code{evaluate()}} \usage{ flush_console() } @@ -14,8 +14,7 @@ be immediately written out, and more importantly, the \code{text} handler (specified in the \code{output_handler} argument of \code{evaluate()}) will be called, which makes it possible for users to know it when the code produces text output using the handler. -} -\note{ + This function is supposed to be called inside \code{evaluate()} (e.g. either a direct \code{evaluate()} call or in \pkg{knitr} code chunks). } diff --git a/tests/testthat/test-flush-console.R b/tests/testthat/test-flush-console.R new file mode 100644 index 00000000..f2ac5262 --- /dev/null +++ b/tests/testthat/test-flush-console.R @@ -0,0 +1,27 @@ + + +test_that("flush_console() is a null op by default", { + expect_no_error(flush_console()) +}) + +test_that("can set and restore output handler", { + f <- function() message("Hi") + old <- set_output_handler(function() message("Hi")) + expect_equal(.env$output_handler, f) + expect_equal(old, NULL) + + expect_message(flush_console(), "Hi") + old2 <- set_output_handler(old) + expect_equal(old2, f) +}) + +test_that("can use flush_console() inside evaluate", { + test <- function() { + cat("hi") + flush_console() + cat("bye") + } + ev <- evaluate("test()") + expect_equal(ev[[2]], "hi") + expect_equal(ev[[3]], "bye") +}) From ba8f5bcca412a44ccd19cd9f0bc98f623cff2fe7 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 17:18:03 +0100 Subject: [PATCH 07/10] Eliminate enclose argument (#137) This eliminates one argument we need to thread through multiple function calls by creating a new environment with appropriate parent. (I doubt anyone actually uses this argument but it should be a safe transformation) --- R/eval.R | 14 ++++++-------- R/utils.R | 2 ++ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/R/eval.R b/R/eval.R index fe994ea3..8be1f5c7 100644 --- a/R/eval.R +++ b/R/eval.R @@ -64,8 +64,8 @@ evaluate <- function(input, return(list(source, err)) } - if (is.null(enclos)) { - enclos <- if (is.list(envir) || is.pairlist(envir)) parent.frame() else baseenv() + if (is.list(envir)) { + envir <- list2env(envir, parent = enclos %||% parent.frame()) } if (new_device) { @@ -99,7 +99,6 @@ evaluate <- function(input, exprs = parsed$expr[[i]], src = parsed$src[[i]], envir = envir, - enclos = enclos, debug = debug, last = i == length(out), use_try = stop_on_error != 2L, @@ -128,7 +127,6 @@ evaluate <- function(input, evaluate_top_level_expression <- function(exprs, src = NULL, envir = parent.frame(), - enclos = NULL, debug = FALSE, last = FALSE, use_try = FALSE, @@ -242,7 +240,7 @@ evaluate_top_level_expression <- function(exprs, srcindex <- length(output) time <- timing_fn(handle( ev <- withCallingHandlers( - withVisible(eval_with_user_handlers(expr, envir, enclos, user_handlers)), + withVisible(eval_with_user_handlers(expr, envir, user_handlers)), warning = wHandler, error = eHandler, message = mHandler @@ -274,9 +272,9 @@ evaluate_top_level_expression <- function(exprs, output } -eval_with_user_handlers <- function(expr, envir, enclos, calling_handlers) { +eval_with_user_handlers <- function(expr, envir, calling_handlers) { if (!length(calling_handlers)) { - return(eval(expr, envir, enclos)) + return(eval(expr, envir)) } if (!is.list(calling_handlers)) { @@ -285,7 +283,7 @@ eval_with_user_handlers <- function(expr, envir, enclos, calling_handlers) { call <- as.call(c( quote(withCallingHandlers), - quote(eval(expr, envir, enclos)), + quote(eval(expr, envir)), calling_handlers )) diff --git a/R/utils.R b/R/utils.R index c46dda23..1df0c3e6 100644 --- a/R/utils.R +++ b/R/utils.R @@ -2,3 +2,5 @@ defer <- function(expr, frame = parent.frame(), after = FALSE) { thunk <- as.call(list(function() expr)) do.call(on.exit, list(thunk, TRUE, after), envir = frame) } + +`%||%` <- function(a, b) if (is.null(a)) b else a From 82345d3c90e996616ebfb956ac15456d30628d95 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 17:45:00 +0100 Subject: [PATCH 08/10] Always needs to capture output before handling conditions (#139) Fixes #28 --- NEWS.md | 2 ++ R/eval.R | 41 +++++++++++++------------- tests/testthat/test-eval.R | 59 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 21 deletions(-) diff --git a/NEWS.md b/NEWS.md index c1fbf046..f7b4415e 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # evaluate (development version) +* `evaluate()` now correctly captures plots created before messages/warnings/errors (#28). + # evaluate 0.24.0 * The `source` output handler can now take two arguments (the unparsed `src` diff --git a/R/eval.R b/R/eval.R index 8be1f5c7..b4828f9f 100644 --- a/R/eval.R +++ b/R/eval.R @@ -155,7 +155,7 @@ evaluate_top_level_expression <- function(exprs, output <- list(source) dev <- dev.cur() - handle_output <- function(plot = FALSE, incomplete_plots = FALSE) { + handle_output <- function(plot = TRUE, incomplete_plots = FALSE) { # if dev.cur() has changed, we should not record plots any more plot <- plot && identical(dev, dev.cur()) out <- w$get_new(plot, incomplete_plots, @@ -166,22 +166,14 @@ evaluate_top_level_expression <- function(exprs, local_output_handler(function() handle_output(FALSE)) # Hooks to capture plot creation - capture_plot <- function() { - handle_output(TRUE) - } hook_list <- list( - persp = capture_plot, - before.plot.new = capture_plot, - before.grid.newpage = capture_plot + persp = handle_output, + before.plot.new = handle_output, + before.grid.newpage = handle_output ) set_hooks(hook_list) on.exit(remove_hooks(hook_list), add = TRUE) - handle_condition <- function(cond) { - handle_output() - output <<- c(output, list(cond)) - } - # Handlers for warnings, errors and messages wHandler <- function(wn) { if (log_warning) { @@ -193,21 +185,28 @@ evaluate_top_level_expression <- function(exprs, if (getOption("warn") >= 2) return() if (keep_warning && getOption("warn") >= 0) { - handle_condition(wn) + handle_output() + output <<- c(output, list(wn)) output_handler$warning(wn) } invokeRestart("muffleWarning") } - eHandler <- if (use_try) function(e) { - handle_condition(e) - output_handler$error(e) - } else identity - mHandler <- if (is.na(keep_message)) identity else function(m) { - if (keep_message) { - handle_condition(m) + eHandler <- function(e) { + handle_output() + if (use_try) { + output <<- c(output, list(e)) + output_handler$error(e) + } + } + mHandler <- function(m) { + handle_output() + if (isTRUE(keep_message)) { + output <<- c(output, list(m)) output_handler$message(m) + invokeRestart("muffleMessage") + } else if (isFALSE(keep_message)) { + invokeRestart("muffleMessage") } - invokeRestart("muffleMessage") } ev <- list(value = NULL, visible = FALSE) diff --git a/tests/testthat/test-eval.R b/tests/testthat/test-eval.R index 1879d495..50eb0244 100644 --- a/tests/testthat/test-eval.R +++ b/tests/testthat/test-eval.R @@ -1,3 +1,62 @@ +test_that("all condition handlers first capture output", { + test <- function(){ + plot(1, main = "one") + message("this is an message!") + plot(2, main = "two") + warning("this is a warning") + plot(3, main = "three") + stop("this is an error") + } + expect_equal( + classes(evaluate("test()")), + c( + "source", + "recordedplot", + "simpleMessage", + "recordedplot", + "simpleWarning", + "recordedplot", + "simpleError" + ) + ) +}) + +test_that("all three states of keep_warning work as expected", { + test <- function() { + warning("Hi!") + } + + # warning captured in output + expect_no_warning(ev <- evaluate("test()", keep_warning = TRUE)) + expect_equal(classes(ev), c("source", "simpleWarning")) + + # warning propagated + expect_warning(ev <- evaluate("test()", keep_warning = NA), "Hi") + expect_equal(classes(ev), "source") + + # warning ignored + expect_no_warning(ev <- evaluate("test()", keep_warning = FALSE)) + expect_equal(classes(ev), "source") +}) + +test_that("all three states of keep_message work as expected", { + test <- function() { + message("Hi!") + } + + # message captured in output + expect_no_message(ev <- evaluate("test()", keep_message = TRUE)) + expect_equal(classes(ev), c("source", "simpleMessage")) + + # message propagated + expect_message(ev <- evaluate("test()", keep_message = NA), "Hi") + expect_equal(classes(ev), "source") + + # message ignored + expect_no_message(ev <- evaluate("test()", keep_message = FALSE)) + expect_equal(classes(ev), "source") +}) + test_that("can evaluate expressions of all lengths", { source <- " # a comment From 00c80e11252ddaa92c7d4b30e4b0ca00234b6d26 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 17:45:08 +0100 Subject: [PATCH 09/10] Centralise value handling in one place (#140) One small step to moving more logic into the output handler and out of `evaluate()`. --- R/eval.R | 10 ++-------- R/output.R | 24 +++++++++++++++++++++--- man/new_output_handler.Rd | 9 ++++++--- tests/testthat/test-evaluate.R | 2 ++ 4 files changed, 31 insertions(+), 14 deletions(-) diff --git a/R/eval.R b/R/eval.R index b4828f9f..d0edb842 100644 --- a/R/eval.R +++ b/R/eval.R @@ -216,7 +216,6 @@ evaluate_top_level_expression <- function(exprs, } else { handle <- force } - value_handler <- output_handler$value if (include_timing) { timing_fn <- function(x) system.time(x)[1:3] } else { @@ -234,7 +233,6 @@ evaluate_top_level_expression <- function(exprs, user_handlers <- output_handler$calling_handlers - multi_args <- length(formals(value_handler)) > 1 for (expr in exprs) { srcindex <- length(output) time <- timing_fn(handle( @@ -249,14 +247,10 @@ evaluate_top_level_expression <- function(exprs, if (!is.null(time)) attr(output[[srcindex]]$src, 'timing') <- time - # If visible or the value handler has multi args, process and capture output - if (ev$visible || multi_args) { + if (show_value(output_handler, ev$visible)) { pv <- list(value = NULL, visible = FALSE) - value_fun <- if (multi_args) value_handler else { - function(x, visible) value_handler(x) - } handle(pv <- withCallingHandlers(withVisible( - value_fun(ev$value, ev$visible) + handle_value(output_handler, ev$value, ev$visible) ), warning = wHandler, error = eHandler, message = mHandler)) handle_output(TRUE) # If the return value is visible, save the value to the output diff --git a/R/output.R b/R/output.R index c5e79cd0..8161fdac 100644 --- a/R/output.R +++ b/R/output.R @@ -43,6 +43,23 @@ new_source <- function(src, call, handler = NULL) { } } +# If the output handler has two arguments, then the user has opted into +# handling the value regardless of whether it's not visible. +show_value <- function(handler, visible) { + visible || length(formals(handler$value)) > 1 +} + +handle_value <- function(handler, value, visible) { + n_args <- length(formals(handler$value)) + if (n_args == 1) { + handler$value(value) + } else if (n_args == 2) { + handler$value(value, visible) + } else { + stop("Value output handler must have one or two arguments") + } +} + classes <- function(x) vapply(x, function(x) class(x)[1], character(1)) render <- function(x) if (isS4(x)) methods::show(x) else print(x) @@ -78,9 +95,10 @@ render <- function(x) if (isS4(x)) methods::show(x) else print(x) #' @param message Function to handle [message()] output. #' @param warning Function to handle [warning()] output. #' @param error Function to handle [stop()] output. -#' @param value Function to handle the values returned from evaluation. If it -#' only has one argument, only visible values are handled; if it has more -#' arguments, the second argument indicates whether the value is visible. +#' @param value Function to handle the values returned from evaluation. +#' * If it has one argument, it called on visible values. +#' * If it has two arguments, it handles all values, with the second +#' argument indicating whether or not the value is visible. #' @param calling_handlers List of [calling handlers][withCallingHandlers]. #' These handlers have precedence over the exiting handler installed #' by [evaluate()] when `stop_on_error` is set to 0. diff --git a/man/new_output_handler.Rd b/man/new_output_handler.Rd index 798a7a3b..d11e58d6 100644 --- a/man/new_output_handler.Rd +++ b/man/new_output_handler.Rd @@ -37,9 +37,12 @@ drop the source from the output.} \item{error}{Function to handle \code{\link[=stop]{stop()}} output.} -\item{value}{Function to handle the values returned from evaluation. If it -only has one argument, only visible values are handled; if it has more -arguments, the second argument indicates whether the value is visible.} +\item{value}{Function to handle the values returned from evaluation. +\itemize{ +\item If it has one argument, it called on visible values. +\item If it has two arguments, it handles all values, with the second +argument indicating whether or not the value is visible. +}} \item{calling_handlers}{List of \link[=withCallingHandlers]{calling handlers}. These handlers have precedence over the exiting handler installed diff --git a/tests/testthat/test-evaluate.R b/tests/testthat/test-evaluate.R index 857c297b..5cdab3a0 100644 --- a/tests/testthat/test-evaluate.R +++ b/tests/testthat/test-evaluate.R @@ -114,6 +114,8 @@ test_that("invisible values can also be saved if value handler has two arguments handler <- new_output_handler(value = function(x, visible) { x # always returns a visible value }) + expect_true(show_value(handler, FALSE)) + ev <- evaluate("x<-1:10", output_handler = handler) expect_equal(classes(ev), c("source", "integer")) }) From 31521799a4256e8570c703756eca3350d973fc10 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 18 Jun 2024 17:47:36 +0100 Subject: [PATCH 10/10] use_package_doc() (#142) --- R/evaluate-package.R | 6 ++++++ man/evaluate-package.Rd | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 R/evaluate-package.R create mode 100644 man/evaluate-package.Rd diff --git a/R/evaluate-package.R b/R/evaluate-package.R new file mode 100644 index 00000000..a65cf643 --- /dev/null +++ b/R/evaluate-package.R @@ -0,0 +1,6 @@ +#' @keywords internal +"_PACKAGE" + +## usethis namespace: start +## usethis namespace: end +NULL diff --git a/man/evaluate-package.Rd b/man/evaluate-package.Rd new file mode 100644 index 00000000..449df5c3 --- /dev/null +++ b/man/evaluate-package.Rd @@ -0,0 +1,41 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/evaluate-package.R +\docType{package} +\name{evaluate-package} +\alias{evaluate-package} +\title{evaluate: Parsing and Evaluation Tools that Provide More Details than the Default} +\description{ +Parsing and evaluation tools that make it easy to recreate the command line behaviour of R. +} +\seealso{ +Useful links: +\itemize{ + \item \url{https://evaluate.r-lib.org/} + \item \url{https://github.com/r-lib/evaluate} + \item Report bugs at \url{https://github.com/r-lib/evaluate/issues} +} + +} +\author{ +\strong{Maintainer}: Hadley Wickham \email{hadley@posit.co} + +Authors: +\itemize{ + \item Yihui Xie (\href{https://orcid.org/0000-0003-0645-5666}{ORCID}) +} + +Other contributors: +\itemize{ + \item Michael Lawrence [contributor] + \item Thomas Kluyver [contributor] + \item Jeroen Ooms [contributor] + \item Barret Schloerke [contributor] + \item Adam Ryczkowski [contributor] + \item Hiroaki Yutani [contributor] + \item Michel Lang [contributor] + \item Karolis Koncevičius [contributor] + \item Posit Software, PBC [copyright holder, funder] +} + +} +\keyword{internal}