From f391a296a7434f1f3941f6bfd4ddbb38dda2ae08 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Wed, 20 Mar 2024 09:30:18 +0000 Subject: [PATCH 1/2] Make tests more isolated. Some tests were calling `orderly_run` without an `envir` parameter, which ends up running the report in the global environment. This pollutes that namespace and might affect subsequent tests. One particular issue I was hitting was a test depending on the global `data` name from `utils`, while other tests were defining a global variable with that (unfortunately very common) name. I've added a `envir = new.env()` parameter to all the `orderly_run` calls I could find, and changed the test that depended on `data` to use the fully qualified name `utils::base`. Running the whole test suite now leaves an unmodified global environment: ```r ls() # character(0) devtools::test() ls() # character(0) ``` --- tests/testthat/test-cleanup.R | 2 +- tests/testthat/test-init.R | 6 +++-- tests/testthat/test-parameters.R | 6 ++--- tests/testthat/test-run.R | 42 ++++++++++++++++++-------------- 4 files changed, 32 insertions(+), 24 deletions(-) diff --git a/tests/testthat/test-cleanup.R b/tests/testthat/test-cleanup.R index e05c4133..c9b62c11 100644 --- a/tests/testthat/test-cleanup.R +++ b/tests/testthat/test-cleanup.R @@ -217,7 +217,7 @@ test_that("Don't call cleanup on an active packet", { append_lines(file.path(path_src, "data.R"), "orderly2::orderly_cleanup_status()") expect_error( - orderly_run_quietly("data", root = path), + orderly_run_quietly("data", root = path, envir = new.env()), "Don't call 'orderly2::orderly_cleanup_status()' from a running packet", fixed = TRUE) }) diff --git a/tests/testthat/test-init.R b/tests/testthat/test-init.R index 7b2d628d..3a510bab 100644 --- a/tests/testthat/test-init.R +++ b/tests/testthat/test-init.R @@ -47,7 +47,8 @@ test_that("can initialise a repo with orderly but no .outpack directory", { base <- basename(path) unlink(file.path(path, ".outpack"), recursive = TRUE) err <- expect_error( - withr::with_dir(parent, orderly_run_quietly("data", root = base)), + withr::with_dir(parent, + orderly_run_quietly("data", root = base, envir = new.env())), sprintf("orderly directory '%s' not initialise", base)) expect_equal( err$body, @@ -60,7 +61,8 @@ test_that("can initialise a repo with orderly but no .outpack directory", { root <- root_open(path, FALSE, TRUE) expect_true(is_directory(file.path(path, ".outpack"))) - id <- withr::with_dir(parent, orderly_run_quietly("data", root = base)) + id <- withr::with_dir(parent, + orderly_run_quietly("data", root = base, envir = new.env())) expect_type(id, "character") }) diff --git a/tests/testthat/test-parameters.R b/tests/testthat/test-parameters.R index 753ef7c2..6a4c82a2 100644 --- a/tests/testthat/test-parameters.R +++ b/tests/testthat/test-parameters.R @@ -144,14 +144,14 @@ test_that("parameters must be atomic scalars", { "x" = "Values must be character, numeric or boolean, but were not for:", "*" = "a")) err <- expect_error( - check_parameters(list(a = data, b = 2), list(a = NULL, b = NULL)), + check_parameters(list(a = utils::data, b = 2), list(a = NULL, b = NULL)), "Invalid parameter value\\b") expect_equal( err$body, c("x" = "Values must be character, numeric or boolean, but were not for:", "*" = "a")) err <- expect_error( - check_parameters(list(a = data, b = 2 + 1i), list(a = NULL, b = NULL)), + check_parameters(list(a = utils::data, b = 2 + 1i), list(a = NULL, b = NULL)), "Invalid parameter values\\b") expect_equal( err$body, @@ -177,7 +177,7 @@ test_that("defaults must be valid", { expect_equal(err$body, c("x" = "Values must be scalar, but were not for:", "*" = "a")) err <- expect_error( - static_orderly_parameters(list(a = data)), + static_orderly_parameters(list(a = utils::data)), "Invalid parameter default") expect_equal( err$body, diff --git a/tests/testthat/test-run.R b/tests/testthat/test-run.R index 13fac677..8a9da639 100644 --- a/tests/testthat/test-run.R +++ b/tests/testthat/test-run.R @@ -375,7 +375,8 @@ test_that("with strict mode, only declared files are copied, running fails", { writeLines(c("orderly2::orderly_strict_mode()", code), path_src) err <- suppressWarnings(tryCatch(read.csv("data.csv"), error = identity)) expect_error( - suppressWarnings(orderly_run_quietly("implicit", root = path)), + suppressWarnings( + orderly_run_quietly("implicit", root = path, envir = new.env())), err$message, fixed = TRUE) }) @@ -390,7 +391,7 @@ test_that("with strict mode, indicate unknown files as potential artefacts", { code), path_src) res <- testthat::evaluate_promise( - id <- orderly_run("implicit", root = path)) + id <- orderly_run("implicit", root = path, envir = new.env())) expect_match(res$messages, "orderly produced unexpected files:\n - mygraph.png", all = FALSE) @@ -404,7 +405,7 @@ test_that("without strict mode, detect modified files", { path <- test_prepare_orderly_example("implicit") file.create(file.path(path, "src", "implicit", "mygraph.png")) res <- testthat::evaluate_promise( - id <- orderly_run("implicit", root = path)) + id <- orderly_run("implicit", root = path, envir = new.env())) expect_match( res$messages, "inputs modified; these are probably artefacts:\n - mygraph.png", @@ -425,7 +426,7 @@ test_that("disallow multiple calls to strict mode", { code), path_src) expect_error( - orderly_run_quietly("implicit", root = path), + orderly_run_quietly("implicit", root = path, envir = new.env()), "orderly function 'orderly_strict_mode' can only be used at the top level") writeLines(c("orderly2::orderly_strict_mode()", @@ -433,7 +434,7 @@ test_that("disallow multiple calls to strict mode", { code), path_src) expect_error( - orderly_run_quietly("implicit", root = path), + orderly_run_quietly("implicit", root = path, envir = new.env()), "Only one call to 'orderly2::orderly_strict_mode' is allowed") }) @@ -573,9 +574,9 @@ test_that("don't copy artefacts over when not needed", { test_that("can pull resources programmatically", { path <- test_prepare_orderly_example("programmatic-resource") id1 <- orderly_run_quietly("programmatic-resource", list(use = "a"), - root = path) + root = path, envir = new.env()) id2 <- orderly_run_quietly("programmatic-resource", list(use = "b"), - root = path) + root = path, envir = new.env()) meta1 <- orderly_metadata(id1, root = path) meta2 <- orderly_metadata(id2, root = path) @@ -598,9 +599,9 @@ test_that("can pull resources programmatically, strictly", { "programmatic-resource.R") prepend_lines(path_src, "orderly2::orderly_strict_mode()") id1 <- orderly_run_quietly("programmatic-resource", list(use = "a"), - root = path) + root = path, envir = new.env()) id2 <- orderly_run_quietly("programmatic-resource", list(use = "b"), - root = path) + root = path, envir = new.env()) meta1 <- meta <- orderly_metadata(id1, root = path) meta2 <- meta <- orderly_metadata(id2, root = path) @@ -697,7 +698,9 @@ test_that("Can select location when querying dependencies for a report", { for (nm in c("us", "prod", "dev")) { path[[nm]] <- test_prepare_orderly_example(c("data", "depends")) if (nm != "us") { - ids[[nm]] <- orderly_run_quietly("data", root = path[[nm]]) + ids[[nm]] <- orderly_run_quietly("data", root = path[[nm]], + envir = new.env()) + orderly_location_add(nm, "path", list(path = path[[nm]]), root = path[["us"]]) orderly_location_pull_metadata(nm, root = path[["us"]]) @@ -707,24 +710,27 @@ test_that("Can select location when querying dependencies for a report", { } } ## Run extra local copy - this is the most recent. - ids[["us"]] <- orderly_run_quietly("data", root = path[["us"]]) + ids[["us"]] <- orderly_run_quietly("data", root = path[["us"]], + envir = new.env()) ## Without locations we prefer the local one: - id1 <- orderly_run_quietly("depends", root = path[["us"]]) + id1 <- orderly_run_quietly("depends", root = path[["us"]], envir = new.env()) expect_equal(orderly_metadata(id1, path[["us"]])$depends$packet, ids[["us"]]) ## Filter to only allow prod: id2 <- orderly_run_quietly("depends", search_options = list(location = "prod"), - root = path[["us"]]) + root = path[["us"]], + envir = new.env()) expect_equal(orderly_metadata(id2, path[["us"]])$depends$packet, ids[["prod"]]) ## Allow any location: id3 <- orderly_run_quietly("depends", search_options = list(location = c("prod", "dev")), - root = path[["us"]]) + root = path[["us"]], + envir = new.env()) expect_equal(orderly_metadata(id3, path[["us"]])$depends$packet, ids[["dev"]]) }) @@ -1047,7 +1053,7 @@ test_that("can use quote for queries queries", { path_src <- file.path(path, "src", "depends", "depends.R") src <- readLines(path_src) writeLines(sub('"latest"', "quote(latest())", src, fixed = TRUE), path_src) - id2 <- orderly_run_quietly("depends", root = path) + id2 <- orderly_run_quietly("depends", root = path, envir = new.env()) expect_equal(orderly_metadata(id2, root = path)$depends$packet, id1[[2]]) }) @@ -1166,7 +1172,7 @@ test_that("can read about assigned resources", { code <- c(code, 'writeLines(r, "resources.txt")') writeLines(code, file.path(path_src, "directories.R")) - id <- orderly_run_quietly("directories", root = path) + id <- orderly_run_quietly("directories", root = path, envir = new.env()) expect_setequal( readLines(file.path(path, "archive", "directories", id, "resources.txt")), c("data/a.csv", "data/b.csv")) @@ -1197,7 +1203,7 @@ test_that("can read about assigned shared resources", { code <- c(code, 'saveRDS(r, "resources.rds")') writeLines(code, file.path(path_src, "shared-dir.R")) - id <- orderly_run_quietly("shared-dir", root = path) + id <- orderly_run_quietly("shared-dir", root = path, envir = new.env()) r <- readRDS(file.path(path, "archive", "shared-dir", id, "resources.rds")) expect_equal( r, @@ -1227,7 +1233,7 @@ test_that("can read about dependencies", { code <- c(code, 'saveRDS(r, "depends.rds")') writeLines(code, file.path(path_src, "depends.R")) - id2 <- orderly_run_quietly("depends", root = path) + id2 <- orderly_run_quietly("depends", root = path, envir = new.env()) r <- readRDS(file.path(path, "archive", "depends", id2, "depends.rds")) expect_equal(r$id, id1) expect_equal(r$name, "data") From 0e60e3de3bd0f4c043c5b8f2487ea49d3a9b4d94 Mon Sep 17 00:00:00 2001 From: Paul Lietar Date: Wed, 20 Mar 2024 09:36:25 +0000 Subject: [PATCH 2/2] Fix formatting --- tests/testthat/test-parameters.R | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/testthat/test-parameters.R b/tests/testthat/test-parameters.R index 6a4c82a2..d87158d6 100644 --- a/tests/testthat/test-parameters.R +++ b/tests/testthat/test-parameters.R @@ -151,7 +151,8 @@ test_that("parameters must be atomic scalars", { c("x" = "Values must be character, numeric or boolean, but were not for:", "*" = "a")) err <- expect_error( - check_parameters(list(a = utils::data, b = 2 + 1i), list(a = NULL, b = NULL)), + check_parameters(list(a = utils::data, b = 2 + 1i), + list(a = NULL, b = NULL)), "Invalid parameter values\\b") expect_equal( err$body,