From 5288c2e77942835a3db19714dfa381c829c9ee9b Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Tue, 25 May 2021 23:52:05 +0200 Subject: [PATCH 01/11] don't change relative to files, but just tempdir --- R/testing.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/testing.R b/R/testing.R index 9436eb963..21bc52f21 100644 --- a/R/testing.R +++ b/R/testing.R @@ -103,14 +103,14 @@ run_test_impl <- function(path_executable, path_candidate_temp <- fs::path(tempdir, basename(path_candidate)) fs::file_copy(path_candidate, path_candidate_temp, overwrite = TRUE) path_candidate_temp <- withr::with_dir( - fs::path_dir(path_candidate_temp), + tempdir, file_transformer(path_candidate_temp) ) withr::defer(fs::file_delete(path_candidate_temp)) path_stderr <- tempfile() path_stdout <- tempfile() status <- withr::with_dir( - fs::path_dir(path_candidate_temp), + tempdir, { files <- fs::path_file(path_candidate_temp) # https://r.789695.n4.nabble.com/Error-message-Rscript-should-not-be-used-without-a-path-td4748071.html @@ -124,7 +124,7 @@ run_test_impl <- function(path_executable, path_temp <- tempfile() fs::file_copy(path_candidate, path_temp) path_temp <- withr::with_dir( - fs::path_dir(path_candidate_temp), + tempdir, file_transformer(path_temp) ) reference <- readLines(path_temp) From 79704a100ca1dd91c6ddb09624bd21aadf939ed4 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Wed, 26 May 2021 21:40:17 +0200 Subject: [PATCH 02/11] files can now be placed outside the test root similar to `copy` --- R/testing.R | 131 +++++++++++++++++++++++++++++---------- man/copy_artifacts.Rd | 13 ++++ man/hook_state_assert.Rd | 23 +++++++ man/hook_state_create.Rd | 22 +++++++ man/run_test.Rd | 15 +++-- man/run_test_impl.Rd | 11 ++-- 6 files changed, 166 insertions(+), 49 deletions(-) create mode 100644 man/copy_artifacts.Rd create mode 100644 man/hook_state_assert.Rd create mode 100644 man/hook_state_create.Rd diff --git a/R/testing.R b/R/testing.R index 21bc52f21..9aef67f06 100644 --- a/R/testing.R +++ b/R/testing.R @@ -17,7 +17,9 @@ #' the message we expect. To check changed file content, we set `error_msg` to #' `NA`. #' @param hook_name The name of the hook in `bin/`. -#' @param file_name The file to test in `tests/in` (without extension). +#' @param file_name The file to test in `tests/in` (without extension). Can be +#' a named vector of length one where the name is the target location relative +#' to the temporary location and the value is the source of the file. #' @param suffix The suffix of `file_name`. #' @param file_transformer A function that takes the file names as input and is #' ran right before the hook script is invoked, returning the path to the @@ -45,7 +47,9 @@ run_test <- function(hook_name, package = "precommit" ) test_ancestor <- testthat::test_path("in", file_name) + names(test_ancestor) <- names(file_name) path_candidate <- paste0(test_ancestor, suffix) + names(path_candidate) <- names(test_ancestor) run_test_impl( path_executable, path_candidate[1], error_msg = error_msg, @@ -62,13 +66,10 @@ run_test <- function(hook_name, #' @param path_executable The path to the executable bash script. #' @param path_candidate The path to a file that should be modified by the #' executable. -#' @param copy Path with files to copy to the temp directory where the test -#' is run. If the target destination relative to the temp dir where the hook -#' is tested is not identical to the path from where a file should be copied, -#' you can pass a named vector. The name is the target directory relative to -#' the temp directory where the hook is executed (the temp directory will be -#' the working directory at that time) and the value is the path that points -#' to the place where the artifact is currently stored. +#' @param copy Path with artifact files to copy to the temp directory root where +#' the test is run. If you don't target the root, this can be a named vector +#' of length one where the name is the target location relative to the +#' temporary location and the value is the source of the file. #' @param error_msg An expected error message. If no error is expected, this #' can be `NULL`. In that case, the `comparator` is applied. #' @param msg The expected stdout message. If `NULL`, this check is omitted. @@ -86,40 +87,83 @@ run_test_impl <- function(path_executable, env) { expect_success <- is.null(error_msg) tempdir <- fs::dir_create(fs::file_temp()) - if (!is.null(copy)) { - if (is.null(names(copy))) { - # not namesm take directory name - new_dirs <- fs::path(tempdir, fs::path_dir(copy)) - fs::dir_create(new_dirs) - paths_copy <- fs::path(new_dirs, fs::path_file(copy)) - } else { - paths_copy <- fs::path(tempdir, names(copy)) - new_dirs <- fs::path_dir(paths_copy) - fs::dir_create(new_dirs) - } - withr::defer(fs::file_delete(paths_copy)) - fs::file_copy(copy, paths_copy, overwrite = TRUE) - } - path_candidate_temp <- fs::path(tempdir, basename(path_candidate)) + copy_artifacts(copy, tempdir) + # if name set use this, otherwise put in root + path_candidate_temp <- fs::path( + tempdir, + ifelse(is.null(names(path_candidate)), basename(path_candidate), + names(path_candidate) + ) + ) + fs::dir_create(fs::path_dir(path_candidate_temp)) fs::file_copy(path_candidate, path_candidate_temp, overwrite = TRUE) path_candidate_temp <- withr::with_dir( tempdir, file_transformer(path_candidate_temp) ) - withr::defer(fs::file_delete(path_candidate_temp)) path_stderr <- tempfile() path_stdout <- tempfile() - status <- withr::with_dir( + exit_status <- hook_state_create( tempdir, - { - files <- fs::path_file(path_candidate_temp) - # https://r.789695.n4.nabble.com/Error-message-Rscript-should-not-be-used-without-a-path-td4748071.html - system2(paste0(Sys.getenv("R_HOME"), "/bin/Rscript"), - args = c(path_executable, cmd_args, files), - stderr = path_stderr, stdout = path_stdout, env = env - ) - } + path_candidate_temp, + path_executable, + cmd_args, + files, + path_stdout, + path_stderr, + env ) + hook_state_assert( + path_candidate, + tempdir, + path_candidate_temp, + file_transformer, + path_stdout, + path_stderr, + expect_success, + error_msg, + msg, + exit_status + ) +} + + +#' Create a hook state +#' +#' Runs the hook script to create a hook state, i.e. exit code, transformed +#' files and emitted messages of the hook run. +#' @keywords internal +hook_state_create <- function(tempdir, + path_candidate_temp, + path_executable, + cmd_args, + files, + path_stdout, + path_stderr, + env) { + withr::local_dir(tempdir) + files <- fs::path_rel(path_candidate_temp, tempdir) + # https://r.789695.n4.nabble.com/Error-message-Rscript-should-not-be-used-without-a-path-td4748071.html + system2(paste0(Sys.getenv("R_HOME"), "/bin/Rscript"), + args = c(path_executable, cmd_args, files), + stderr = path_stderr, stdout = path_stdout, env = env + ) +} + +#' Check if the hook produced what you want +#' +#' Match the resulting state after the hook run with the expected state +#' @keywords internal +hook_state_assert <- function(path_candidate, + tempdir, + path_candidate_temp, + file_transformer, + path_stdout, + path_stderr, + expect_success, + error_msg, + msg, + exit_status) { candidate <- readLines(path_candidate_temp) path_temp <- tempfile() fs::file_copy(path_candidate, path_temp) @@ -131,7 +175,7 @@ run_test_impl <- function(path_executable, if (expect_success) { # file not changed + no stderr contents <- readLines(path_stderr) - if (status != 0) { + if (exit_status != 0) { testthat::fail("Expected: No error. Found:", contents) } testthat::expect_equivalent(candidate, reference) @@ -227,3 +271,22 @@ generate_uninstalled_pkg_name <- function(n = 10) { generate_uninstalled_pkg_call <- function(n = 10) { paste0(generate_uninstalled_pkg_name(n), "::x") } + + +#' Copy some file to the test directory that must be present, but are not +#' passed to the hook as a file argument. +copy_artifacts <- function(copy, tempdir) { + if (!is.null(copy)) { + if (is.null(names(copy))) { + # not namesm take directory name + new_dirs <- fs::path(tempdir, fs::path_dir(copy)) + fs::dir_create(new_dirs) + paths_copy <- fs::path(new_dirs, fs::path_file(copy)) + } else { + paths_copy <- fs::path(tempdir, names(copy)) + new_dirs <- fs::path_dir(paths_copy) + fs::dir_create(new_dirs) + } + fs::file_copy(copy, paths_copy, overwrite = TRUE) + } +} diff --git a/man/copy_artifacts.Rd b/man/copy_artifacts.Rd new file mode 100644 index 000000000..e37df3771 --- /dev/null +++ b/man/copy_artifacts.Rd @@ -0,0 +1,13 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/testing.R +\name{copy_artifacts} +\alias{copy_artifacts} +\title{Copy some file to the test directory that must be present, but are not +passed to the hook as a file argument.} +\usage{ +copy_artifacts(copy, tempdir) +} +\description{ +Copy some file to the test directory that must be present, but are not +passed to the hook as a file argument. +} diff --git a/man/hook_state_assert.Rd b/man/hook_state_assert.Rd new file mode 100644 index 000000000..7953e40fb --- /dev/null +++ b/man/hook_state_assert.Rd @@ -0,0 +1,23 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/testing.R +\name{hook_state_assert} +\alias{hook_state_assert} +\title{Check if the hook produced what you want} +\usage{ +hook_state_assert( + path_candidate, + tempdir, + path_candidate_temp, + file_transformer, + path_stdout, + path_stderr, + expect_success, + error_msg, + msg, + exit_status +) +} +\description{ +Match the resulting state after the hook run with the expected state +} +\keyword{internal} diff --git a/man/hook_state_create.Rd b/man/hook_state_create.Rd new file mode 100644 index 000000000..f5a85c6de --- /dev/null +++ b/man/hook_state_create.Rd @@ -0,0 +1,22 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/testing.R +\name{hook_state_create} +\alias{hook_state_create} +\title{Create a hook state} +\usage{ +hook_state_create( + tempdir, + path_candidate_temp, + path_executable, + cmd_args, + files, + path_stdout, + path_stderr, + env +) +} +\description{ +Runs the hook script to create a hook state, i.e. exit code, transformed +files and emitted messages of the hook run. +} +\keyword{internal} diff --git a/man/run_test.Rd b/man/run_test.Rd index f8da6b20e..189564e9f 100644 --- a/man/run_test.Rd +++ b/man/run_test.Rd @@ -19,7 +19,9 @@ run_test( \arguments{ \item{hook_name}{The name of the hook in \verb{bin/}.} -\item{file_name}{The file to test in \verb{tests/in} (without extension).} +\item{file_name}{The file to test in \verb{tests/in} (without extension). Can be +a named vector of length one where the name is the target location relative +to the temporary location and the value is the source of the file.} \item{suffix}{The suffix of \code{file_name}.} @@ -31,13 +33,10 @@ can be \code{NULL}. In that case, the \code{comparator} is applied.} \item{cmd_args}{More arguments passed to the file. Pre-commit handles it as described \href{https://pre-commit.com/#arguments-pattern-in-hooks}{here}.} -\item{copy}{Path with files to copy to the temp directory where the test -is run. If the target destination relative to the temp dir where the hook -is tested is not identical to the path from where a file should be copied, -you can pass a named vector. The name is the target directory relative to -the temp directory where the hook is executed (the temp directory will be -the working directory at that time) and the value is the path that points -to the place where the artifact is currently stored.} +\item{copy}{Path with artifact files to copy to the temp directory root where +the test is run. If you don't target the root, this can be a named vector +of length one where the name is the target location relative to the +temporary location and the value is the source of the file.} \item{file_transformer}{A function that takes the file names as input and is ran right before the hook script is invoked, returning the path to the diff --git a/man/run_test_impl.Rd b/man/run_test_impl.Rd index fcfc37e82..c7315f658 100644 --- a/man/run_test_impl.Rd +++ b/man/run_test_impl.Rd @@ -29,13 +29,10 @@ can be \code{NULL}. In that case, the \code{comparator} is applied.} \item{cmd_args}{More arguments passed to the file. Pre-commit handles it as described \href{https://pre-commit.com/#arguments-pattern-in-hooks}{here}.} -\item{copy}{Path with files to copy to the temp directory where the test -is run. If the target destination relative to the temp dir where the hook -is tested is not identical to the path from where a file should be copied, -you can pass a named vector. The name is the target directory relative to -the temp directory where the hook is executed (the temp directory will be -the working directory at that time) and the value is the path that points -to the place where the artifact is currently stored.} +\item{copy}{Path with artifact files to copy to the temp directory root where +the test is run. If you don't target the root, this can be a named vector +of length one where the name is the target location relative to the +temporary location and the value is the source of the file.} \item{env}{The environment variables to set with \code{\link[base:system2]{base::system2()}}.} } From f0ef2b283a7d9d9d7d4ad01cf0ac5fa7989fac81 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Wed, 26 May 2021 21:41:27 +0200 Subject: [PATCH 03/11] test roxygenize --- inst/WORDLIST | 1 + tests/testthat/in/DESCRIPTION-no-deps.dcf | 6 ++++++ tests/testthat/in/roxygenize.R | 5 +++++ tests/testthat/test-hooks.R | 18 ++++++++++++++++++ 4 files changed, 30 insertions(+) create mode 100644 tests/testthat/in/DESCRIPTION-no-deps.dcf create mode 100644 tests/testthat/in/roxygenize.R diff --git a/inst/WORDLIST b/inst/WORDLIST index 65cf2ef51..c52202916 100644 --- a/inst/WORDLIST +++ b/inst/WORDLIST @@ -80,6 +80,7 @@ pkgapi pkgdown pkgload pre +prec precommit precommithooks proj diff --git a/tests/testthat/in/DESCRIPTION-no-deps.dcf b/tests/testthat/in/DESCRIPTION-no-deps.dcf new file mode 100644 index 000000000..354649d78 --- /dev/null +++ b/tests/testthat/in/DESCRIPTION-no-deps.dcf @@ -0,0 +1,6 @@ +Package: prec3 +Title: Pre commit hooks +Version: 1.0.0 +Author: Lorenz Walthert +Maintainer: Lorenz Walthert +Encoding: UTF-8 diff --git a/tests/testthat/in/roxygenize.R b/tests/testthat/in/roxygenize.R new file mode 100644 index 000000000..e042492d3 --- /dev/null +++ b/tests/testthat/in/roxygenize.R @@ -0,0 +1,5 @@ +#' This is a file +#' +#' @param text This is a parameter. +#' @name flie +NULL diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 3442184c6..3ad9f38d5 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -140,6 +140,7 @@ run_test("deps-in-desc", suffix = "-success.R", error_msg = NULL, copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) + # fail (call to library that is not in description) run_test("deps-in-desc", suffix = "-fail.R", error_msg = "Dependency check failed", @@ -242,3 +243,20 @@ run_test( "lintr", suffix = "-fail.R", cmd_args = "--warn_only", error_msg = NULL ) + +### . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .. +### roxygenize #### +run_test("roxygenize", + file_name = c("R/roxygenize.R" = "roxygenize.R"), + suffix = "", + error_msg = NULL, + msg = "Writing flie.Rd", + copy = c( + "DESCRIPTION" = test_path("in/DESCRIPTION-no-deps.dcf") + ), + file_transformer = function(files) { + git2r::init() + git2r::add(path = files) + files + } +) From 2dee8ff8824df0515047130135236c320f3836de Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Wed, 26 May 2021 22:56:56 +0200 Subject: [PATCH 04/11] fix status --- R/testing.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/testing.R b/R/testing.R index 9aef67f06..ab041435b 100644 --- a/R/testing.R +++ b/R/testing.R @@ -201,7 +201,7 @@ hook_state_assert <- function(path_candidate, paste(contents, collapse = "\n"), error_msg, fixed = TRUE ) - testthat::expect_false(status == 0) + testthat::expect_false(exit_status == 0) } } } From ca7ca8c30536201f624aac21fad7eb786f7cf885 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Wed, 26 May 2021 23:12:39 +0200 Subject: [PATCH 05/11] rename copy -> artifacts --- R/testing.R | 31 ++++++++++++++++--------------- man/copy_artifacts.Rd | 8 +++++++- man/hook_state_create.Rd | 1 - man/run_test.Rd | 4 ++-- man/run_test_impl.Rd | 4 ++-- tests/testthat/test-hooks.R | 26 +++++++++++++------------- 6 files changed, 40 insertions(+), 34 deletions(-) diff --git a/R/testing.R b/R/testing.R index ab041435b..63e03bd79 100644 --- a/R/testing.R +++ b/R/testing.R @@ -38,7 +38,7 @@ run_test <- function(hook_name, error_msg = NULL, msg = NULL, cmd_args = NULL, - copy = NULL, + artifacts = NULL, file_transformer = function(files) files, env = character()) { withr::local_envvar(list(R_PRECOMMIT_HOOK_ENV = "1")) @@ -55,7 +55,7 @@ run_test <- function(hook_name, error_msg = error_msg, msg = msg, cmd_args = cmd_args, - copy = copy, + artifacts = artifacts, file_transformer = file_transformer, env = env ) @@ -66,7 +66,7 @@ run_test <- function(hook_name, #' @param path_executable The path to the executable bash script. #' @param path_candidate The path to a file that should be modified by the #' executable. -#' @param copy Path with artifact files to copy to the temp directory root where +#' @param artifacts Path with artifact files to copy to the temp directory root where #' the test is run. If you don't target the root, this can be a named vector #' of length one where the name is the target location relative to the #' temporary location and the value is the source of the file. @@ -82,12 +82,12 @@ run_test_impl <- function(path_executable, error_msg, msg, cmd_args, - copy, + artifacts, file_transformer, env) { expect_success <- is.null(error_msg) tempdir <- fs::dir_create(fs::file_temp()) - copy_artifacts(copy, tempdir) + copy_artifacts(artifacts, tempdir) # if name set use this, otherwise put in root path_candidate_temp <- fs::path( tempdir, @@ -108,7 +108,6 @@ run_test_impl <- function(path_executable, path_candidate_temp, path_executable, cmd_args, - files, path_stdout, path_stderr, env @@ -137,7 +136,6 @@ hook_state_create <- function(tempdir, path_candidate_temp, path_executable, cmd_args, - files, path_stdout, path_stderr, env) { @@ -275,18 +273,21 @@ generate_uninstalled_pkg_call <- function(n = 10) { #' Copy some file to the test directory that must be present, but are not #' passed to the hook as a file argument. -copy_artifacts <- function(copy, tempdir) { - if (!is.null(copy)) { - if (is.null(names(copy))) { +#' @param artifacts Artifacts to copy. +#' @param tempdir The temporary directory. +#' @keywords internal +copy_artifacts <- function(artifacts, tempdir) { + if (!is.null(artifacts)) { + if (is.null(names(artifacts))) { # not namesm take directory name - new_dirs <- fs::path(tempdir, fs::path_dir(copy)) + new_dirs <- fs::path(tempdir, fs::path_dir(artifacts)) fs::dir_create(new_dirs) - paths_copy <- fs::path(new_dirs, fs::path_file(copy)) + paths_artifacts <- fs::path(new_dirs, fs::path_file(artifacts)) } else { - paths_copy <- fs::path(tempdir, names(copy)) - new_dirs <- fs::path_dir(paths_copy) + paths_artifacts <- fs::path(tempdir, names(artifacts)) + new_dirs <- fs::path_dir(paths_artifacts) fs::dir_create(new_dirs) } - fs::file_copy(copy, paths_copy, overwrite = TRUE) + fs::file_copy(artifacts, paths_artifacts, overwrite = TRUE) } } diff --git a/man/copy_artifacts.Rd b/man/copy_artifacts.Rd index e37df3771..4f774f244 100644 --- a/man/copy_artifacts.Rd +++ b/man/copy_artifacts.Rd @@ -5,9 +5,15 @@ \title{Copy some file to the test directory that must be present, but are not passed to the hook as a file argument.} \usage{ -copy_artifacts(copy, tempdir) +copy_artifacts(artifacts, tempdir) +} +\arguments{ +\item{artifacts}{Artifacts to copy.} + +\item{tempdir}{The temporary directory.} } \description{ Copy some file to the test directory that must be present, but are not passed to the hook as a file argument. } +\keyword{internal} diff --git a/man/hook_state_create.Rd b/man/hook_state_create.Rd index f5a85c6de..120573443 100644 --- a/man/hook_state_create.Rd +++ b/man/hook_state_create.Rd @@ -9,7 +9,6 @@ hook_state_create( path_candidate_temp, path_executable, cmd_args, - files, path_stdout, path_stderr, env diff --git a/man/run_test.Rd b/man/run_test.Rd index 189564e9f..37e6a6786 100644 --- a/man/run_test.Rd +++ b/man/run_test.Rd @@ -11,7 +11,7 @@ run_test( error_msg = NULL, msg = NULL, cmd_args = NULL, - copy = NULL, + artifacts = NULL, file_transformer = function(files) files, env = character() ) @@ -33,7 +33,7 @@ can be \code{NULL}. In that case, the \code{comparator} is applied.} \item{cmd_args}{More arguments passed to the file. Pre-commit handles it as described \href{https://pre-commit.com/#arguments-pattern-in-hooks}{here}.} -\item{copy}{Path with artifact files to copy to the temp directory root where +\item{artifacts}{Path with artifact files to copy to the temp directory root where the test is run. If you don't target the root, this can be a named vector of length one where the name is the target location relative to the temporary location and the value is the source of the file.} diff --git a/man/run_test_impl.Rd b/man/run_test_impl.Rd index c7315f658..87b6879ba 100644 --- a/man/run_test_impl.Rd +++ b/man/run_test_impl.Rd @@ -10,7 +10,7 @@ run_test_impl( error_msg, msg, cmd_args, - copy, + artifacts, file_transformer, env ) @@ -29,7 +29,7 @@ can be \code{NULL}. In that case, the \code{comparator} is applied.} \item{cmd_args}{More arguments passed to the file. Pre-commit handles it as described \href{https://pre-commit.com/#arguments-pattern-in-hooks}{here}.} -\item{copy}{Path with artifact files to copy to the temp directory root where +\item{artifacts}{Path with artifact files to copy to the temp directory root where the test is run. If you don't target the root, this can be a named vector of length one where the name is the target location relative to the temporary location and the value is the source of the file.} diff --git a/tests/testthat/test-hooks.R b/tests/testthat/test-hooks.R index 3ad9f38d5..34ecf644d 100644 --- a/tests/testthat/test-hooks.R +++ b/tests/testthat/test-hooks.R @@ -124,7 +124,7 @@ run_test("spell-check", suffix = "-fail.md", error_msg = "Spell check failed") run_test("spell-check", suffix = "-wordlist-success.md", error_msg = NULL, - copy = c("inst/WORDLIST" = test_path("in/WORDLIST")) + artifacts = c("inst/WORDLIST" = test_path("in/WORDLIST")) ) # success with ignored files @@ -138,32 +138,32 @@ run_test("spell-check", suffix = "-language-success.md", cmd_args = "--lang=en_G # succeed (call to library that is in description) run_test("deps-in-desc", suffix = "-success.R", error_msg = NULL, - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) # fail (call to library that is not in description) run_test("deps-in-desc", suffix = "-fail.R", error_msg = "Dependency check failed", - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) # with ::: run_test("deps-in-desc", "deps-in-desc-dot3", suffix = "-fail.R", error_msg = "Dependency check failed", - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) run_test("deps-in-desc", "deps-in-desc-dot3", suffix = "-success.R", error_msg = NULL, - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) run_test("deps-in-desc", "deps-in-desc-dot3", suffix = "-fail.R", error_msg = NULL, - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")), + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")), cmd_args = "--allow_private_imports" ) @@ -171,13 +171,13 @@ run_test("deps-in-desc", run_test("deps-in-desc", "deps-in-desc", suffix = "-fail.Rmd", error_msg = "Dependency check failed", - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) run_test("deps-in-desc", "deps-in-desc", suffix = "-success.Rmd", error_msg = NULL, - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) @@ -185,13 +185,13 @@ run_test("deps-in-desc", run_test("deps-in-desc", "deps-in-desc", suffix = "-fail.Rnw", error_msg = "Dependency check failed", - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) run_test("deps-in-desc", "deps-in-desc", suffix = "-success.Rnw", error_msg = NULL, - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")) + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")) ) # Rprofile @@ -202,7 +202,7 @@ expect_true(rlang::is_installed("R.cache")) run_test("deps-in-desc", "Rprofile", suffix = "", error_msg = "Dependency check failed", - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")), + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")), file_transformer = function(files) { writeLines("R.cache::findCache", files) fs::file_move( @@ -215,7 +215,7 @@ run_test("deps-in-desc", run_test("deps-in-desc", "Rprofile", suffix = "", error_msg = NULL, - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")), + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")), file_transformer = function(files) { writeLines("utils::head", files) fs::file_move( @@ -251,7 +251,7 @@ run_test("roxygenize", suffix = "", error_msg = NULL, msg = "Writing flie.Rd", - copy = c( + artifacts = c( "DESCRIPTION" = test_path("in/DESCRIPTION-no-deps.dcf") ), file_transformer = function(files) { From 2afd143a5969cae0538fac983d3307432ebb2a74 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Wed, 26 May 2021 23:38:46 +0200 Subject: [PATCH 06/11] simplify naming --- R/testing.R | 28 +++++++--------------------- R/utils.R | 16 ++++++++++++++++ man/ensure_named.Rd | 17 +++++++++++++++++ 3 files changed, 40 insertions(+), 21 deletions(-) create mode 100644 man/ensure_named.Rd diff --git a/R/testing.R b/R/testing.R index 63e03bd79..e1030e937 100644 --- a/R/testing.R +++ b/R/testing.R @@ -46,16 +46,14 @@ run_test <- function(hook_name, fs::path("bin", hook_name), package = "precommit" ) - test_ancestor <- testthat::test_path("in", file_name) - names(test_ancestor) <- names(file_name) - path_candidate <- paste0(test_ancestor, suffix) - names(path_candidate) <- names(test_ancestor) + path_candidate <- paste0(testthat::test_path("in", file_name), suffix) %>% + ensure_named(names(file_name), fs::path_file) run_test_impl( path_executable, path_candidate[1], error_msg = error_msg, msg = msg, cmd_args = cmd_args, - artifacts = artifacts, + artifacts = ensure_named(artifacts), file_transformer = file_transformer, env = env ) @@ -89,12 +87,7 @@ run_test_impl <- function(path_executable, tempdir <- fs::dir_create(fs::file_temp()) copy_artifacts(artifacts, tempdir) # if name set use this, otherwise put in root - path_candidate_temp <- fs::path( - tempdir, - ifelse(is.null(names(path_candidate)), basename(path_candidate), - names(path_candidate) - ) - ) + path_candidate_temp <- fs::path(tempdir, names(path_candidate)) fs::dir_create(fs::path_dir(path_candidate_temp)) fs::file_copy(path_candidate, path_candidate_temp, overwrite = TRUE) path_candidate_temp <- withr::with_dir( @@ -278,16 +271,9 @@ generate_uninstalled_pkg_call <- function(n = 10) { #' @keywords internal copy_artifacts <- function(artifacts, tempdir) { if (!is.null(artifacts)) { - if (is.null(names(artifacts))) { - # not namesm take directory name - new_dirs <- fs::path(tempdir, fs::path_dir(artifacts)) - fs::dir_create(new_dirs) - paths_artifacts <- fs::path(new_dirs, fs::path_file(artifacts)) - } else { - paths_artifacts <- fs::path(tempdir, names(artifacts)) - new_dirs <- fs::path_dir(paths_artifacts) - fs::dir_create(new_dirs) - } + paths_artifacts <- fs::path(tempdir, names(artifacts)) + new_dirs <- fs::path_dir(paths_artifacts) + fs::dir_create(new_dirs) fs::file_copy(artifacts, paths_artifacts, overwrite = TRUE) } } diff --git a/R/utils.R b/R/utils.R index 3f57cae33..d86b4f167 100644 --- a/R/utils.R +++ b/R/utils.R @@ -38,6 +38,22 @@ add_trailing_linebreak <- function(x) { paste0(x, "\n") } +#' Name the input +#' +#' @param x A vector. +#' @param f How to transform the input `x` into a name. +#' @keywords internal +ensure_named <- function(x, candidate_name = NULL, f = identity) { + if (is.null(names(x))) { + if (is.null(candidate_name)) { + names(x) <- f(x) + } else { + names(x) <- candidate_name + } + } + x +} + #' Create the path to the precommit R.cache cache #' diff --git a/man/ensure_named.Rd b/man/ensure_named.Rd new file mode 100644 index 000000000..fb23c811d --- /dev/null +++ b/man/ensure_named.Rd @@ -0,0 +1,17 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/utils.R +\name{ensure_named} +\alias{ensure_named} +\title{Name the input} +\usage{ +ensure_named(x, candidate_name = NULL, f = identity) +} +\arguments{ +\item{x}{A vector.} + +\item{f}{How to transform the input \code{x} into a name.} +} +\description{ +Name the input +} +\keyword{internal} From 1e272861242358df50105d5ed40e37f925f43cb2 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Thu, 27 May 2021 10:57:25 +0200 Subject: [PATCH 07/11] add scratch dir --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 2520579bc..1b7e9f9f9 100644 --- a/.gitignore +++ b/.gitignore @@ -4,3 +4,4 @@ .Ruserdata docs/ inst/doc +scratch/ From 03ae2f995ed8e5820b15645c490f230bf7fd9f92 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Thu, 27 May 2021 14:01:27 +0200 Subject: [PATCH 08/11] fix copy arg --- tests/testthat/test-cache.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-cache.R b/tests/testthat/test-cache.R index ba9c2a365..09b77683a 100644 --- a/tests/testthat/test-cache.R +++ b/tests/testthat/test-cache.R @@ -53,7 +53,7 @@ test_that("CLI API works for roxygenize", { suffix = "-cache-success.R", env = paste0("R_CACHE_ROOTPATH=", R.cache_root), msg = "You can silent this", - copy = c("DESCRIPTION" = test_path("in/DESCRIPTION")), + artifacts = c("DESCRIPTION" = test_path("in/DESCRIPTION")), file_transformer = function(x) { git2r::init() x From b73fa18e00785a1870033a4193b8356d88a89a8a Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Thu, 27 May 2021 14:12:08 +0200 Subject: [PATCH 09/11] install git2r explicitly as a testing dependency --- .github/workflows/hook-tests-legacy.yml | 2 +- .github/workflows/hook-tests.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/hook-tests-legacy.yml b/.github/workflows/hook-tests-legacy.yml index 847b7f3b5..24e193fb8 100644 --- a/.github/workflows/hook-tests-legacy.yml +++ b/.github/workflows/hook-tests-legacy.yml @@ -77,7 +77,7 @@ jobs: if (!requireNamespace("renv", quietly = TRUE)) { install.packages("renv", repos = c(CRAN = "https://cloud.r-project.org")) } - renv::install(c('testthat', 'devtools', 'desc')) + renv::install(c('testthat', 'devtools', 'desc', 'git2r')) renv::install(desc::desc_get_deps()$package) # install all deps renv::install(getwd()) shell: Rscript {0} diff --git a/.github/workflows/hook-tests.yaml b/.github/workflows/hook-tests.yaml index 107b1bd91..b450e9438 100644 --- a/.github/workflows/hook-tests.yaml +++ b/.github/workflows/hook-tests.yaml @@ -85,7 +85,7 @@ jobs: if (!requireNamespace("renv", quietly = TRUE)) { install.packages("renv", repos = c(CRAN = "https://cloud.r-project.org")) } - renv::install(c('testthat', 'devtools')) + renv::install(c('testthat', 'devtools', 'git2r')) # some testing infrastructure we need is in R/testing.R renv::install(getwd()) shell: Rscript {0} From a1b24bc1ad3e2b52969bf544f99c765a47e0af61 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Thu, 27 May 2021 14:59:36 +0200 Subject: [PATCH 10/11] must also use testing utilities from v0.1.3 --- .github/workflows/hook-tests-legacy.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/hook-tests-legacy.yml b/.github/workflows/hook-tests-legacy.yml index 24e193fb8..371aae090 100644 --- a/.github/workflows/hook-tests-legacy.yml +++ b/.github/workflows/hook-tests-legacy.yml @@ -35,6 +35,7 @@ jobs: git fetch origin refs/tags/v0.1.3:refs/tags/v0.1.3 git checkout v0.1.3 -- inst/bin git checkout v0.1.3 -- tests/testthat/test-all.R + git checkout v0.1.3 -- R/testing.R - name: Query dependencies run: | writeLines(sprintf("R-%i.%i", getRversion()$major, getRversion()$minor), ".github/R-version") From 70c5a89259594ec53fdb16a4d6e152e9ec81ceb1 Mon Sep 17 00:00:00 2001 From: Lorenz Walthert Date: Thu, 27 May 2021 15:50:23 +0200 Subject: [PATCH 11/11] add news --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index 115e59c66..00843d669 100644 --- a/NEWS.md +++ b/NEWS.md @@ -58,6 +58,7 @@ installation. Silicon when installed with Homebrew (#240). - The `deps-in-desc` hook now points to the hook argument `--allow_private_imports` when the hook fails due to private imports (#254). +- roxygenize hook is now fully tested (#267). - Hook dependency updates are proposed by an automatic monthly pull request to `lorenzwalthert/precommit`. This does not affect users directly (#430).