From 4503f76006ed74cea6e67318283f284589a77573 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Tue, 22 Jun 2021 12:59:07 -0700 Subject: [PATCH 01/23] Move exercise error checking code into `evaluate_exercise()` from `render_exercise()` --- R/exercise.R | 49 ++++++++++++++++++++++------------ tests/testthat/test-exercise.R | 38 ++++++++++++++++---------- 2 files changed, 56 insertions(+), 31 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 06c360935..10130bc8b 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -318,9 +318,31 @@ evaluate_exercise <- function(exercise, envir, evaluate_global_setup = FALSE) { } # Resolve knitr options for the exercise and setup chunks + # evaluate() #### rmd_results <- withr::with_dir( exercise_dir, - render_exercise(exercise, envir) + tryCatch( + render_exercise(exercise, envir), + error = function(e) { + if (length(exercise$error_check)) { + # Run the condition through an error checker + # (the exercise could be to throw an error!) + checker_feedback <- try_checker( + exercise, "exercise.checker", + check_code = exercise$error_check, + envir_result = e$envir_result, + evaluate_result = e$evaluate_result, + envir_prep = e$envir_prep, + last_value = e$e, + engine = exercise$engine + ) + if (is_exercise_result(checker_feedback)) { + return(checker_feedback) + } + } + exercise_result_error(e$msg) + } + ) ) if (is_exercise_result(rmd_results)) { @@ -533,6 +555,7 @@ render_exercise <- function(exercise, envir) { envir_result <- duplicate_env(envir_prep) # Now render user code for final result + # render() #### rmarkdown::render( input = rmd_file_user, output_format = output_format_exercise(user = TRUE), @@ -548,22 +571,14 @@ render_exercise <- function(exercise, envir) { if (grepl(pattern, msg, fixed = TRUE)) { return(exercise_result_timeout()) } - if (length(exercise$error_check)) { - # Run the condition through an error checker (the exercise could be to throw an error!) - checker_feedback <- try_checker( - exercise, "exercise.checker", - check_code = exercise$error_check, - envir_result = envir_result, - evaluate_result = evaluate_result, - envir_prep = envir_prep, - last_value = e, - engine = exercise$engine - ) - if (is_exercise_result(checker_feedback)) { - return(checker_feedback) - } - } - exercise_result_error(msg) + rlang::abort( + class = "learnr_exercise_result", + envir_result = envir_result, + evaluate_result = evaluate_result, + envir_prep = envir_prep, + last_value = e, + error_message = msg + ) }) if (is_exercise_result(output_file)) { diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index 7fd6612dc..f37c729ac 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -140,11 +140,9 @@ test_that("render_exercise() returns identical envir_prep and envir_result if an error_check = "unevaluated, triggers error_check in render_exercise()" ) - exercise_result <- withr::with_tempdir(render_exercise(exercise, new.env())) - - # the error during render causes a checker evaluation, so we can recover - # the environments from the checker_args returned by the debug checker - exercise_result <- exercise_result$feedback$checker_args + exercise_result <- withr::with_tempdir( + tryCatch(render_exercise(exercise, new.env()), error = identity) + ) expect_s3_class(exercise_result$last_value, "simpleError") expect_equal(conditionMessage(exercise_result$last_value), "boom") @@ -162,11 +160,9 @@ test_that("render_exercise() returns envir_result up to error", { error_check = "unevaluated, triggers error_check in render_exercise()" ) - exercise_result <- withr::with_tempdir(render_exercise(exercise, new.env())) - - # the error during render causes a checker evaluation, so we can recover - # the environments from the checker_args returned by the debug checker - exercise_result <- exercise_result$feedback$checker_args + exercise_result <- withr::with_tempdir( + tryCatch(render_exercise(exercise, new.env()), error = identity) + ) expect_s3_class(exercise_result$last_value, "simpleError") expect_equal(conditionMessage(exercise_result$last_value), "boom") @@ -184,13 +180,17 @@ test_that("render_exercise() with errors and no checker returns exercise result setup_label = "setup-1" ) - exercise_result <- withr::with_tempdir(render_exercise(exercise, new.env())) + exercise_result <- withr::with_tempdir( + tryCatch(render_exercise(exercise, new.env()), error = identity) + ) expect_s3_class(exercise_result, "learnr_exercise_result") expect_identical(exercise_result$error_message, "setup") expect_null(exercise_result$feedback) exercise <- mock_exercise(user_code = "stop('user')") - exercise_result <- withr::with_tempdir(render_exercise(exercise, new.env())) + exercise_result <- withr::with_tempdir( + tryCatch(render_exercise(exercise, new.env()), error = identity) + ) expect_s3_class(exercise_result, "learnr_exercise_result") expect_identical(exercise_result$error_message, "user") expect_null(exercise_result$feedback) @@ -228,8 +228,18 @@ test_that("render_exercise() cleans up exercise_prep files even when setup fails files <- expect_message( withr::with_tempdir({ - before <- dir() - res <- render_exercise(exercise, new.env()) + before <- dir() + e <- tryCatch(render_exercise(exercise, new.env()), error = identity) + res <- try_checker( + exercise, "exercise.checker", + check_code = exercise$error_check, + envir_result = e$envir_result, + evaluate_result = e$evaluate_result, + envir_prep = e$envir_prep, + last_value = e$e, + engine = exercise$engine + ) + list( before = before, before_error = get("dir_setup", res$feedback$checker_args$envir_prep), From 259a0821b78992574879dc29e8cdedd54cf651d3 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Tue, 22 Jun 2021 12:59:57 -0700 Subject: [PATCH 02/23] Remove breadcrumbs --- R/exercise.R | 2 -- 1 file changed, 2 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 10130bc8b..17da23b40 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -318,7 +318,6 @@ evaluate_exercise <- function(exercise, envir, evaluate_global_setup = FALSE) { } # Resolve knitr options for the exercise and setup chunks - # evaluate() #### rmd_results <- withr::with_dir( exercise_dir, tryCatch( @@ -555,7 +554,6 @@ render_exercise <- function(exercise, envir) { envir_result <- duplicate_env(envir_prep) # Now render user code for final result - # render() #### rmarkdown::render( input = rmd_file_user, output_format = output_format_exercise(user = TRUE), From ca144c9f27984ec76803069533c5e992e7482e35 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Wed, 23 Jun 2021 20:40:42 -0700 Subject: [PATCH 03/23] Use more specific error class for render errors --- R/exercise.R | 2 +- tests/testthat/test-exercise.R | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 17da23b40..0c8abbc36 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -570,7 +570,7 @@ render_exercise <- function(exercise, envir) { return(exercise_result_timeout()) } rlang::abort( - class = "learnr_exercise_result", + class = "learnr_render_exercise_error", envir_result = envir_result, evaluate_result = evaluate_result, envir_prep = envir_prep, diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index f37c729ac..7c0f816bb 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -183,7 +183,7 @@ test_that("render_exercise() with errors and no checker returns exercise result exercise_result <- withr::with_tempdir( tryCatch(render_exercise(exercise, new.env()), error = identity) ) - expect_s3_class(exercise_result, "learnr_exercise_result") + expect_s3_class(exercise_result, "learnr_render_exercise_error") expect_identical(exercise_result$error_message, "setup") expect_null(exercise_result$feedback) @@ -191,7 +191,7 @@ test_that("render_exercise() with errors and no checker returns exercise result exercise_result <- withr::with_tempdir( tryCatch(render_exercise(exercise, new.env()), error = identity) ) - expect_s3_class(exercise_result, "learnr_exercise_result") + expect_s3_class(exercise_result, "learnr_render_exercise_error") expect_identical(exercise_result$error_message, "user") expect_null(exercise_result$feedback) }) From c3c38ae44185a7fdf780b34c738fedf7bf941798 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Wed, 23 Jun 2021 20:49:48 -0700 Subject: [PATCH 04/23] Simplify test --- tests/testthat/test-exercise.R | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index 7c0f816bb..d315fca14 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -229,22 +229,12 @@ test_that("render_exercise() cleans up exercise_prep files even when setup fails files <- expect_message( withr::with_tempdir({ before <- dir() - e <- tryCatch(render_exercise(exercise, new.env()), error = identity) - res <- try_checker( - exercise, "exercise.checker", - check_code = exercise$error_check, - envir_result = e$envir_result, - evaluate_result = e$evaluate_result, - envir_prep = e$envir_prep, - last_value = e$e, - engine = exercise$engine - ) + e <- rlang::catch_cnd(render_exercise(exercise, new.env())) list( before = before, - before_error = get("dir_setup", res$feedback$checker_args$envir_prep), - during = res$feedback$checker_result, - after = dir() + during = get("dir_setup", e$envir_prep), + after = dir() ) }), "exercise_prep.Rmd" @@ -253,9 +243,7 @@ test_that("render_exercise() cleans up exercise_prep files even when setup fails # start with nothing expect_identical(files$before, character(0)) # prep file is present while evaluating prep - expect_identical(files$before_error, "exercise_prep.Rmd") - # prep files are cleaned up after error - expect_identical(files$during, character(0)) + expect_identical(files$during, "exercise_prep.Rmd") # nothing in directory after render_exercise() because user code didn't evaluate expect_identical(files$after, character(0)) }) From aa6abd7b3878a9d89a9e3176c25b2bcb58484acb Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Wed, 23 Jun 2021 20:50:41 -0700 Subject: [PATCH 05/23] Use `rlang::catch_cnd()` instead of `tryCatch()` --- tests/testthat/test-exercise.R | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index d315fca14..78729396a 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -141,7 +141,9 @@ test_that("render_exercise() returns identical envir_prep and envir_result if an ) exercise_result <- withr::with_tempdir( - tryCatch(render_exercise(exercise, new.env()), error = identity) + rlang::catch_cnd( + render_exercise(exercise, new.env()), "learnr_render_exercise_error" + ) ) expect_s3_class(exercise_result$last_value, "simpleError") @@ -161,7 +163,9 @@ test_that("render_exercise() returns envir_result up to error", { ) exercise_result <- withr::with_tempdir( - tryCatch(render_exercise(exercise, new.env()), error = identity) + rlang::catch_cnd( + render_exercise(exercise, new.env()), "learnr_render_exercise_error" + ) ) expect_s3_class(exercise_result$last_value, "simpleError") @@ -181,7 +185,9 @@ test_that("render_exercise() with errors and no checker returns exercise result ) exercise_result <- withr::with_tempdir( - tryCatch(render_exercise(exercise, new.env()), error = identity) + rlang::catch_cnd( + render_exercise(exercise, new.env()), "learnr_render_exercise_error" + ) ) expect_s3_class(exercise_result, "learnr_render_exercise_error") expect_identical(exercise_result$error_message, "setup") @@ -189,7 +195,9 @@ test_that("render_exercise() with errors and no checker returns exercise result exercise <- mock_exercise(user_code = "stop('user')") exercise_result <- withr::with_tempdir( - tryCatch(render_exercise(exercise, new.env()), error = identity) + rlang::catch_cnd( + render_exercise(exercise, new.env()), "learnr_render_exercise_error" + ) ) expect_s3_class(exercise_result, "learnr_render_exercise_error") expect_identical(exercise_result$error_message, "user") @@ -229,7 +237,9 @@ test_that("render_exercise() cleans up exercise_prep files even when setup fails files <- expect_message( withr::with_tempdir({ before <- dir() - e <- rlang::catch_cnd(render_exercise(exercise, new.env())) + e <- rlang::catch_cnd( + render_exercise(exercise, new.env()), "learnr_render_exercise_error" + ) list( before = before, From 0cb3dfda34d025f7b09cf0db18c6d11a95be9efc Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Wed, 23 Jun 2021 21:03:17 -0700 Subject: [PATCH 06/23] Run code checks before creating temp dir --- R/exercise.R | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 0c8abbc36..3783850d8 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -294,12 +294,6 @@ evaluate_exercise <- function(exercise, envir, evaluate_global_setup = FALSE) { eval(parse(text = exercise$global_setup), envir = envir) } - - # Setup a temporary directory for rendering the exercise - exercise_dir <- tempfile(pattern = "lnr-ex") - dir.create(exercise_dir) - on.exit(unlink(exercise_dir), add = TRUE) - checker_feedback <- NULL # Run the checker pre-evaluation _if_ there is code checking to do if (length(exercise$code_check)) { @@ -317,6 +311,11 @@ evaluate_exercise <- function(exercise, envir, evaluate_global_setup = FALSE) { } } + # Setup a temporary directory for rendering the exercise + exercise_dir <- tempfile(pattern = "lnr-ex") + dir.create(exercise_dir) + on.exit(unlink(exercise_dir), add = TRUE) + # Resolve knitr options for the exercise and setup chunks rmd_results <- withr::with_dir( exercise_dir, From bcd5dc3ba831bf44d255dc2643201610161dda32 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Wed, 23 Jun 2021 21:08:30 -0700 Subject: [PATCH 07/23] Use `local_dir()` instead of `with_dir()` in `evaluate_exercise()` --- R/exercise.R | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 3783850d8..664841993 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -314,33 +314,31 @@ evaluate_exercise <- function(exercise, envir, evaluate_global_setup = FALSE) { # Setup a temporary directory for rendering the exercise exercise_dir <- tempfile(pattern = "lnr-ex") dir.create(exercise_dir) + withr::local_dir(exercise_dir) on.exit(unlink(exercise_dir), add = TRUE) # Resolve knitr options for the exercise and setup chunks - rmd_results <- withr::with_dir( - exercise_dir, - tryCatch( - render_exercise(exercise, envir), - error = function(e) { - if (length(exercise$error_check)) { - # Run the condition through an error checker - # (the exercise could be to throw an error!) - checker_feedback <- try_checker( - exercise, "exercise.checker", - check_code = exercise$error_check, - envir_result = e$envir_result, - evaluate_result = e$evaluate_result, - envir_prep = e$envir_prep, - last_value = e$e, - engine = exercise$engine - ) - if (is_exercise_result(checker_feedback)) { - return(checker_feedback) - } + rmd_results <- tryCatch( + render_exercise(exercise, envir), + error = function(e) { + if (length(exercise$error_check)) { + # Run the condition through an error checker + # (the exercise could be to throw an error!) + checker_feedback <- try_checker( + exercise, "exercise.checker", + check_code = exercise$error_check, + envir_result = e$envir_result, + evaluate_result = e$evaluate_result, + envir_prep = e$envir_prep, + last_value = e$e, + engine = exercise$engine + ) + if (is_exercise_result(checker_feedback)) { + return(checker_feedback) } - exercise_result_error(e$msg) } - ) + exercise_result_error(e$msg) + } ) if (is_exercise_result(rmd_results)) { From 18b0fa0d49904e621e6c91d94bb0993ce96a759f Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Thu, 24 Jun 2021 12:37:08 -0700 Subject: [PATCH 08/23] Simplify creation of tempdir --- R/exercise.R | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 664841993..c07233835 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -312,10 +312,8 @@ evaluate_exercise <- function(exercise, envir, evaluate_global_setup = FALSE) { } # Setup a temporary directory for rendering the exercise - exercise_dir <- tempfile(pattern = "lnr-ex") - dir.create(exercise_dir) + exercise_dir <- withr::local_tempdir(pattern = "lrn-ex") withr::local_dir(exercise_dir) - on.exit(unlink(exercise_dir), add = TRUE) # Resolve knitr options for the exercise and setup chunks rmd_results <- tryCatch( From 6e19e4adcee28e5f10f6f690130f9fa9b5810ad9 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Thu, 24 Jun 2021 15:03:19 -0700 Subject: [PATCH 09/23] Modify test to check `render_exercise()` and `evaluate_exercise()` --- R/exercise.R | 2 +- tests/testthat/test-exercise.R | 24 ++++++++++++++++++------ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index c07233835..babe362a6 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -328,7 +328,7 @@ evaluate_exercise <- function(exercise, envir, evaluate_global_setup = FALSE) { envir_result = e$envir_result, evaluate_result = e$evaluate_result, envir_prep = e$envir_prep, - last_value = e$e, + last_value = e, engine = exercise$engine ) if (is_exercise_result(checker_feedback)) { diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index 78729396a..a22882c8c 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -140,16 +140,26 @@ test_that("render_exercise() returns identical envir_prep and envir_result if an error_check = "unevaluated, triggers error_check in render_exercise()" ) - exercise_result <- withr::with_tempdir( + render_result <- withr::with_tempdir( rlang::catch_cnd( render_exercise(exercise, new.env()), "learnr_render_exercise_error" ) ) + expect_s3_class(render_result$last_value, "simpleError") + expect_equal(conditionMessage(render_result$last_value), "boom") + expect_identical(render_result$envir_prep, render_result$envir_result) - expect_s3_class(exercise_result$last_value, "simpleError") - expect_equal(conditionMessage(exercise_result$last_value), "boom") - - expect_identical(exercise_result$envir_prep, exercise_result$envir_result) + eval_result <- evaluate_exercise(exercise, new.env()) + expect_s3_class( + eval_result$feedback$checker_args$last_value, "learnr_render_exercise_error" + ) + expect_equal( + eval_result$feedback$checker_args$last_value$error_message, "boom" + ) + expect_identical( + eval_result$feedback$checker_args$envir_prep, + eval_result$feedback$checker_args$envir_result + ) }) test_that("render_exercise() returns envir_result up to error", { @@ -171,7 +181,9 @@ test_that("render_exercise() returns envir_result up to error", { expect_s3_class(exercise_result$last_value, "simpleError") expect_equal(conditionMessage(exercise_result$last_value), "boom") - expect_false(identical(exercise_result$envir_prep, exercise_result$envir_result)) + expect_false( + identical(exercise_result$envir_prep, exercise_result$envir_result) + ) expect_setequal(ls(exercise_result$envir_prep), "x") expect_setequal(ls(exercise_result$envir_result), c("x", "y")) expect_identical(get("y", exercise_result$envir_result), 2) From f04bdd4c5ff8a9680af34cc2c94b144e18f77157 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Fri, 25 Jun 2021 08:57:46 -0700 Subject: [PATCH 10/23] Fix bug in returning error message --- R/exercise.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/exercise.R b/R/exercise.R index babe362a6..52dc2e127 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -335,7 +335,7 @@ evaluate_exercise <- function(exercise, envir, evaluate_global_setup = FALSE) { return(checker_feedback) } } - exercise_result_error(e$msg) + exercise_result_error(e$error_message) } ) From ea1675b379fe0d48cb204188f28641cd1a86dd21 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Fri, 25 Jun 2021 08:58:03 -0700 Subject: [PATCH 11/23] Update tests of `render_exercise()` and transfer tests to `evaluate_exercise()` --- tests/testthat/test-exercise.R | 36 +++++++--------------------------- 1 file changed, 7 insertions(+), 29 deletions(-) diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index a22882c8c..8914dd42f 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -148,18 +148,7 @@ test_that("render_exercise() returns identical envir_prep and envir_result if an expect_s3_class(render_result$last_value, "simpleError") expect_equal(conditionMessage(render_result$last_value), "boom") expect_identical(render_result$envir_prep, render_result$envir_result) - - eval_result <- evaluate_exercise(exercise, new.env()) - expect_s3_class( - eval_result$feedback$checker_args$last_value, "learnr_render_exercise_error" - ) - expect_equal( - eval_result$feedback$checker_args$last_value$error_message, "boom" - ) - expect_identical( - eval_result$feedback$checker_args$envir_prep, - eval_result$feedback$checker_args$envir_result - ) + expect_equal(get("x", render_result$envir_prep), 1) }) test_that("render_exercise() returns envir_result up to error", { @@ -189,30 +178,19 @@ test_that("render_exercise() returns envir_result up to error", { expect_identical(get("y", exercise_result$envir_result), 2) }) -test_that("render_exercise() with errors and no checker returns exercise result error", { +test_that("evaluate_exercise() with errors and no checker includes exercise result error", { exercise <- mock_exercise( user_code = "stop('user')", chunks = list(mock_chunk("setup-1", "stop('setup')")), setup_label = "setup-1" ) - - exercise_result <- withr::with_tempdir( - rlang::catch_cnd( - render_exercise(exercise, new.env()), "learnr_render_exercise_error" - ) - ) - expect_s3_class(exercise_result, "learnr_render_exercise_error") - expect_identical(exercise_result$error_message, "setup") + exercise_result <- evaluate_exercise(exercise, new.env()) + expect_equal(exercise_result$error_message, "setup") expect_null(exercise_result$feedback) - exercise <- mock_exercise(user_code = "stop('user')") - exercise_result <- withr::with_tempdir( - rlang::catch_cnd( - render_exercise(exercise, new.env()), "learnr_render_exercise_error" - ) - ) - expect_s3_class(exercise_result, "learnr_render_exercise_error") - expect_identical(exercise_result$error_message, "user") + exercise <- mock_exercise(user_code = "stop('user')") + exercise_result <- evaluate_exercise(exercise, new.env()) + expect_equal(exercise_result$error_message, "user") expect_null(exercise_result$feedback) }) From 939eda6990178a9e02ecd44f625344fb6328866e Mon Sep 17 00:00:00 2001 From: rossellhayes Date: Fri, 9 Jul 2021 15:45:24 +0000 Subject: [PATCH 12/23] Build docs (GitHub Actions) --- docs/examples.html | 1 - docs/exercises.html | 1 - docs/formats.html | 1 - docs/index.html | 1 - docs/publishing.html | 3 +-- docs/questions.html | 1 - docs/site_libs/header-attrs-2.9/header-attrs.js | 12 ------------ 7 files changed, 1 insertion(+), 19 deletions(-) delete mode 100644 docs/site_libs/header-attrs-2.9/header-attrs.js diff --git a/docs/examples.html b/docs/examples.html index 16f6fc61b..bfc81bf36 100644 --- a/docs/examples.html +++ b/docs/examples.html @@ -13,7 +13,6 @@ Examples - diff --git a/docs/exercises.html b/docs/exercises.html index 458449eb7..96c6da82e 100644 --- a/docs/exercises.html +++ b/docs/exercises.html @@ -13,7 +13,6 @@ Exercises - diff --git a/docs/formats.html b/docs/formats.html index ae228d1c4..6f0c838d5 100644 --- a/docs/formats.html +++ b/docs/formats.html @@ -13,7 +13,6 @@ Formats - diff --git a/docs/index.html b/docs/index.html index 3694d276c..34d538b24 100644 --- a/docs/index.html +++ b/docs/index.html @@ -13,7 +13,6 @@ Interactive Tutorials for R - diff --git a/docs/publishing.html b/docs/publishing.html index 22a84bb23..9e98abc7a 100644 --- a/docs/publishing.html +++ b/docs/publishing.html @@ -13,7 +13,6 @@ Publishing - @@ -388,7 +387,7 @@

R Package

install.packages("learnr")
 learnr::run_tutorial("introduction", package = "mypackage")
-

Exercise Checkers

+

Exercise Checkers

Note that if your tutorial performs exercise checking via an external package then you should be sure to add the package you use for checking as an Imports dependency of your package so it’s installed automatically along with your package.

Note that it’s likely that the learnr package will eventually include or depend on another package that provides checking functions. For the time being though explicit installation of external checking packages is a requirement.

diff --git a/docs/questions.html b/docs/questions.html index b7e1498bb..5f5c1c672 100644 --- a/docs/questions.html +++ b/docs/questions.html @@ -13,7 +13,6 @@ Questions - diff --git a/docs/site_libs/header-attrs-2.9/header-attrs.js b/docs/site_libs/header-attrs-2.9/header-attrs.js deleted file mode 100644 index dd57d92e0..000000000 --- a/docs/site_libs/header-attrs-2.9/header-attrs.js +++ /dev/null @@ -1,12 +0,0 @@ -// Pandoc 2.9 adds attributes on both header and div. We remove the former (to -// be compatible with the behavior of Pandoc < 2.8). -document.addEventListener('DOMContentLoaded', function(e) { - var hs = document.querySelectorAll("div.section[class*='level'] > :first-child"); - var i, h, a; - for (i = 0; i < hs.length; i++) { - h = hs[i]; - if (!/^h[1-6]$/i.test(h.tagName)) continue; // it should be a header h1-h6 - a = h.attributes; - while (a.length > 0) h.removeAttribute(a[0].name); - } -}); From c0ba8a1d4d31431ac9d691e0b66f8b8cdaa0a288 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 12:13:59 -0400 Subject: [PATCH 13/23] Restore data directory feature --- R/exercise.R | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/R/exercise.R b/R/exercise.R index 22f12f3c8..ce2bed9e8 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -327,6 +327,11 @@ evaluate_exercise <- function( # Setup a temporary directory for rendering the exercise exercise_dir <- withr::local_tempdir(pattern = "lrn-ex") + + # Copy files from data directory into exercise + copy_data_dir(data_dir, exercise_dir) + + # Move into the temp exercise directory for evaluation and checking withr::local_dir(exercise_dir) # Resolve knitr options for the exercise and setup chunks From f56e82e0865b794a63d2086d7ec51629d9d6c138 Mon Sep 17 00:00:00 2001 From: Alex Rossell Hayes Date: Fri, 9 Jul 2021 09:22:22 -0700 Subject: [PATCH 14/23] Fix merge conflict --- R/exercise.R | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 22f12f3c8..948d4d7a0 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -326,31 +326,38 @@ evaluate_exercise <- function( } # Setup a temporary directory for rendering the exercise - exercise_dir <- withr::local_tempdir(pattern = "lrn-ex") - withr::local_dir(exercise_dir) + exercise_dir <- tempfile(pattern = "lnr-ex") + dir.create(exercise_dir) + on.exit(unlink(exercise_dir), add = TRUE) + + # Copy files from data directory into exercise + copy_data_dir(data_dir, exercise_dir) # Resolve knitr options for the exercise and setup chunks - rmd_results <- tryCatch( - render_exercise(exercise, envir), - error = function(e) { - if (length(exercise$error_check)) { - # Run the condition through an error checker - # (the exercise could be to throw an error!) - checker_feedback <- try_checker( - exercise, "exercise.checker", - check_code = exercise$error_check, - envir_result = e$envir_result, - evaluate_result = e$evaluate_result, - envir_prep = e$envir_prep, - last_value = e, - engine = exercise$engine - ) - if (is_exercise_result(checker_feedback)) { - return(checker_feedback) + rmd_results <- withr::with_dir( + exercise_dir, + tryCatch( + render_exercise(exercise, envir), + error = function(e) { + if (length(exercise$error_check)) { + # Run the condition through an error checker + # (the exercise could be to throw an error!) + checker_feedback <- try_checker( + exercise, "exercise.checker", + check_code = exercise$error_check, + envir_result = e$envir_result, + evaluate_result = e$evaluate_result, + envir_prep = e$envir_prep, + last_value = e, + engine = exercise$engine + ) + if (is_exercise_result(checker_feedback)) { + return(checker_feedback) + } } + exercise_result_error(e$error_message) } - exercise_result_error(e$error_message) - } + ) ) if (is_exercise_result(rmd_results)) { From c0149cf4364f5ee4f7f202bb2a73f8a77073f0a7 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 12:36:47 -0400 Subject: [PATCH 15/23] Restore withr::local_tempdir() --- R/exercise.R | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 68579be30..ce2bed9e8 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -326,9 +326,7 @@ evaluate_exercise <- function( } # Setup a temporary directory for rendering the exercise - exercise_dir <- tempfile(pattern = "lnr-ex") - dir.create(exercise_dir) - on.exit(unlink(exercise_dir), add = TRUE) + exercise_dir <- withr::local_tempdir(pattern = "lrn-ex") # Copy files from data directory into exercise copy_data_dir(data_dir, exercise_dir) From 3365c8ec38302de5ec779eb293a73463172f9313 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 12:44:39 -0400 Subject: [PATCH 16/23] Fix comment about purpose of render_exercise() --- R/exercise.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/exercise.R b/R/exercise.R index ce2bed9e8..459ab5874 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -334,7 +334,7 @@ evaluate_exercise <- function( # Move into the temp exercise directory for evaluation and checking withr::local_dir(exercise_dir) - # Resolve knitr options for the exercise and setup chunks + # Evaluate the submitted code by rendering the exercise in a special .Rmd rmd_results <- tryCatch( render_exercise(exercise, envir), error = function(e) { From c5e8a892e352b1e675b89b712f68b70e31ab9eab Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 12:47:15 -0400 Subject: [PATCH 17/23] Rename error object in render_exercise error --- R/exercise.R | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 459ab5874..fcd1e0c8f 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -337,24 +337,24 @@ evaluate_exercise <- function( # Evaluate the submitted code by rendering the exercise in a special .Rmd rmd_results <- tryCatch( render_exercise(exercise, envir), - error = function(e) { + error = function(err_render) { if (length(exercise$error_check)) { # Run the condition through an error checker # (the exercise could be to throw an error!) checker_feedback <- try_checker( exercise, "exercise.checker", check_code = exercise$error_check, - envir_result = e$envir_result, - evaluate_result = e$evaluate_result, - envir_prep = e$envir_prep, - last_value = e, + envir_result = err_render$envir_result, + evaluate_result = err_render$evaluate_result, + envir_prep = err_render$envir_prep, + last_value = err_render, engine = exercise$engine ) if (is_exercise_result(checker_feedback)) { return(checker_feedback) } } - exercise_result_error(e$error_message) + exercise_result_error(err_render$error_message) } ) From ee19db24ce06684c3d0882d02f5ccbe5c9af01fc Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 13:14:06 -0400 Subject: [PATCH 18/23] Add test for exercises that exceed the timelimt --- R/exercise.R | 1 + tests/testthat/test-exercise.R | 26 ++++++++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/R/exercise.R b/R/exercise.R index fcd1e0c8f..fab8c12dd 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -595,6 +595,7 @@ render_exercise <- function(exercise, envir) { }) if (is_exercise_result(output_file)) { + # this only happens when the render result is a timeout error return(output_file) } diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index 54d33f7b7..9d8a9d36d 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -764,3 +764,29 @@ test_that("env vars are protected from both user and author modification", { # evaluate_exercise() restores the TEST option after checking too expect_equal(res$after_eval, "APP") }) + + +# Timelimit --------------------------------------------------------------- + +test_that("Exercise timelimit error is returned when exercise takes too long", { + skip_on_cran() + + ex <- mock_exercise(user_code = "Sys.sleep(3)", exercise.timelimit = 1) + + make_evaluator <- setup_forked_evaluator_factory(max_forked_procs = 1) + evaluator <- make_evaluator( + evaluate_exercise(ex, new.env()), + timelimit = ex$options$exercise.timelimit + ) + + evaluator$start() + while (!evaluator$completed()) { + Sys.sleep(1) + } + res <- evaluator$result() + + expect_s3_class(res, "learnr_exercise_result") + expect_true(res$timeout_exceeded) + expect_match(res$error_message, "permitted timelimit") + expect_match(as.character(res$html_output), "alert-danger") +}) From 9dab33ffbd45d7c3f472161a385fcca781afcd61 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 13:31:23 -0400 Subject: [PATCH 19/23] More specific tests for presence of error_check code --- R/exercise.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index fab8c12dd..441ecf787 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -338,9 +338,9 @@ evaluate_exercise <- function( rmd_results <- tryCatch( render_exercise(exercise, envir), error = function(err_render) { - if (length(exercise$error_check)) { - # Run the condition through an error checker - # (the exercise could be to throw an error!) + if (!is.null(exercise$error_check) && any(nzchar(exercise$error_check))) { + # Check the error thrown by the submitted code when there's error + # checking: the exercise could be to throw an error! checker_feedback <- try_checker( exercise, "exercise.checker", check_code = exercise$error_check, From bd095bade35bdcf2dea1aa30b2435f8a1a42e0b1 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 15:26:19 -0400 Subject: [PATCH 20/23] Standardize exercise checking-related code to length-1 string --- R/exercise.R | 31 ++++++++++++++++++++++++------- tests/testthat/test-exercise.R | 20 ++++++++++++++++++++ 2 files changed, 44 insertions(+), 7 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 441ecf787..3609e3557 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -264,7 +264,7 @@ upgrade_exercise <- function(exercise, require_items = NULL) { # returns NULL if everything is okay, otherwise a character string describing # the reason the validation check failed. validate_exercise <- function(exercise, require_items = NULL) { -required_names <- c("code", "label", "options", "chunks", require_items) + required_names <- c("code", "label", "options", "chunks", require_items) missing_names <- setdiff(required_names, names(exercise)) if (length(missing_names)) { return(paste("Missing exercise items:", paste(missing_names, collapse = ", "))) @@ -273,6 +273,20 @@ required_names <- c("code", "label", "options", "chunks", require_items) NULL } +standardize_exercise_code <- function(exercise) { + for (type in c("error_check", "code_check", "check", "code", "global_setup")) { + if (inherits(exercise[[type]], "AsIs")) { + next + } + if (is.null(exercise[[type]]) || !length(exercise[[type]])) { + exercise[[type]] <- "" + next + } + exercise[[type]] <- str_trim(paste0(exercise[[type]], collapse = "\n")) + } + exercise +} + # evaluate an exercise and return a list containing output and dependencies # @param evaluate_global_setup - If `FALSE`, will not evaluate the global setup # code. Instead, it just concatenates the exercise- specific setup code and @@ -295,11 +309,14 @@ evaluate_exercise <- function( require_items = if (evaluate_global_setup) "global_setup" ) + # standardize exercise code to single string (code, *check, global_setup) + exercise <- standardize_exercise_code(exercise) + i18n_set_language_option(exercise$tutorial$language) # return immediately and clear visible results # do not consider this an exercise submission - if (!nzchar(str_trim(paste0(exercise$code, collapse = "\n")))) { + if (!nzchar(exercise$code)) { # " " since html_output needs to pass a req() return(exercise_result(html_output = " ")) } @@ -309,8 +326,8 @@ evaluate_exercise <- function( } checker_feedback <- NULL - # Run the checker pre-evaluation _if_ there is code checking to do - if (length(exercise$code_check)) { + # Check the code pre-evaluation, if code_check is provided + if (nzchar(exercise$code_check)) { checker_feedback <- try_checker( exercise, "exercise.checker", check_code = exercise$code_check, @@ -338,7 +355,7 @@ evaluate_exercise <- function( rmd_results <- tryCatch( render_exercise(exercise, envir), error = function(err_render) { - if (!is.null(exercise$error_check) && any(nzchar(exercise$error_check))) { + if (nzchar(exercise$error_check)) { # Check the error thrown by the submitted code when there's error # checking: the exercise could be to throw an error! checker_feedback <- try_checker( @@ -363,7 +380,7 @@ evaluate_exercise <- function( } # Run the checker post-evaluation (for checking code results) - if (length(exercise$check)) { + if (nzchar(exercise$check)) { checker_feedback <- try_checker( exercise, "exercise.checker", check_code = exercise$check, @@ -375,7 +392,7 @@ evaluate_exercise <- function( ) } - # include any checker feedback with the exercise results + # Return checker feedback (if any) with the exercise results exercise_result( feedback = checker_feedback$feedback, html_output = rmd_results$html_output diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index 9d8a9d36d..d7395cdea 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -321,6 +321,26 @@ test_that("serialized exercises produce equivalent evaluate_exercise() results", ) }) +test_that("standardize_exercise_result() ensures top-level code is length-1 string", { + ex <- standardize_exercise_code( + list( + code = c("a", "b"), + check = character(), + code_check = c(" ", " ", "\t\t\t"), + global_setup = c( + "", + "def return_one():", + "\treturn 1", + "" + ) + ) + ) + + expect_equal(ex$code, "a\nb") + expect_equal(ex$check, "") + expect_equal(ex$code_check, "") + expect_equal(ex$global_setup, "def return_one():\n\treturn 1") +}) # exercise_result() ------------------------------------------------------- From a4030f05048f40118b825507ed199ca5dae7cac7 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 15:48:30 -0400 Subject: [PATCH 21/23] Skip timelimit test on windows --- tests/testthat/test-exercise.R | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/test-exercise.R b/tests/testthat/test-exercise.R index d7395cdea..d11dd0988 100644 --- a/tests/testthat/test-exercise.R +++ b/tests/testthat/test-exercise.R @@ -790,6 +790,7 @@ test_that("env vars are protected from both user and author modification", { test_that("Exercise timelimit error is returned when exercise takes too long", { skip_on_cran() + skip_on_os("windows") ex <- mock_exercise(user_code = "Sys.sleep(3)", exercise.timelimit = 1) From be678f45253f432bd17fe9a27670c86dc67bf253 Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 16:24:17 -0400 Subject: [PATCH 22/23] Factor out `standardize_code()` We may want to use it in other places --- R/exercise.R | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/R/exercise.R b/R/exercise.R index 3609e3557..901b1c423 100644 --- a/R/exercise.R +++ b/R/exercise.R @@ -273,17 +273,19 @@ validate_exercise <- function(exercise, require_items = NULL) { NULL } -standardize_exercise_code <- function(exercise) { - for (type in c("error_check", "code_check", "check", "code", "global_setup")) { - if (inherits(exercise[[type]], "AsIs")) { - next - } - if (is.null(exercise[[type]]) || !length(exercise[[type]])) { - exercise[[type]] <- "" - next - } - exercise[[type]] <- str_trim(paste0(exercise[[type]], collapse = "\n")) +standardize_code <- function(code) { + if (inherits(code, "AsIs")) { + return(code) + } + if (is.null(code) || !length(code)) { + return("") } + str_trim(paste0(code, collapse = "\n")) +} + +standardize_exercise_code <- function(exercise) { + ex_code_items <- c("error_check", "code_check", "check", "code", "global_setup") + exercise[ex_code_items] <- lapply(exercise[ex_code_items], standardize_code) exercise } From 287aca3cf79fa3facdf41c87f6c3854a261e42ac Mon Sep 17 00:00:00 2001 From: Garrick Aden-Buie Date: Fri, 9 Jul 2021 16:26:53 -0400 Subject: [PATCH 23/23] Add NEWS item --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 9268171e8..1cfccdc45 100644 --- a/NEWS.md +++ b/NEWS.md @@ -47,6 +47,7 @@ learnr (development version) * `exercise_result()` no longer combines the code output and feedback; this now happens just before presenting the exercise result to the user ([#522](https://github.com/rstudio/learnr/pull/522)). * Correct/incorrect question markers are now configurable via CSS. You can change or style these markers using the `.tutorial-question .question-final .correct::before` and `.tutorial-qusetion .question-final .incorrect::before` selectors. A new helper function, `finalize_question()`, can be used to apply the `.question-final` class to custom learnr questions. ([#531](https://github.com/rstudio/learnr/pull/531)) * `options()` and environment variables are now reset after rendering exercises so changes made by user input or checking code cannot affect other exercises. ([#542](https://github.com/rstudio/learnr/pull/542)) +* Exercise checking is now conducted in the same temporary directory where exercises are evaluated. ([#544](https://github.com/rstudio/learnr/pull/544/)) ## Bug fixes