diff --git a/NAMESPACE b/NAMESPACE index c83fa7ba8..426dbf1b4 100644 --- a/NAMESPACE +++ b/NAMESPACE @@ -24,6 +24,7 @@ S3method(print,comparison) S3method(print,expectation) S3method(print,mismatch_character) S3method(print,mismatch_numeric) +S3method(print,testthat_hint) S3method(print,testthat_results) S3method(snapshot_replay,character) S3method(snapshot_replay,condition) diff --git a/NEWS.md b/NEWS.md index c1c21a602..14c09be3c 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # testthat (development version) +* The hints generated by `expect_snapshot()` and `expect_snapshot_file()` now include the path to the package, if its not in the current working directory (#1577). +* `expect_snapshot_file()` now clearly errors if the `path` doesnt exist (#2191). * `expect_snapshot_file()` now considers `.json` to be a text file (#1593). * `expect_snapshot_file()` now shows differences for text files (#1593). * The failure messages for all `expect_` functions have been rewritten to first state what was expected and then what was actually received (#2142). diff --git a/R/local.R b/R/local.R index 1f8b88377..f7cc22bc0 100644 --- a/R/local.R +++ b/R/local.R @@ -177,14 +177,15 @@ local_test_directory <- function(path, package = NULL, .env = parent.frame()) { # Set edition before changing working directory in case path is relative local_edition(find_edition(path, package), .env = .env) - withr::local_dir( - path, - .local_envir = .env - ) + # Capture current working directory so we can use for relative paths + wd <- getwd() + + withr::local_dir(path, .local_envir = .env) withr::local_envvar( R_TESTS = "", TESTTHAT = "true", TESTTHAT_PKG = package, + TESTTHAT_WD = wd, .local_envir = .env ) } diff --git a/R/snapshot-file.R b/R/snapshot-file.R index 4da38f96b..95a031073 100644 --- a/R/snapshot-file.R +++ b/R/snapshot-file.R @@ -109,6 +109,9 @@ expect_snapshot_file <- function( lab <- quo_label(enquo(path)) check_string(path) + if (!file.exists(path)) { + cli::cli_abort("{.path {path}} doesn't exist.") + } check_string(name) check_bool(cran) check_variant(variant) @@ -157,15 +160,10 @@ expect_snapshot_file <- function( } file <- snapshotter$file - if (in_check_reporter()) { - hint <- "" - } else { - hint <- snapshot_review_hint(file, name, is_text = is_text) - } if (!equal) { if (is_text) { - base <- paste0(c(snapshotter$snap_dir, file, variant), collapse = "/") + base <- paste0(c(snapshotter$snap_dir, variant, file), collapse = "/") old_path <- paste0(c(base, name), collapse = "/") new_path <- paste0(c(base, new_name(name)), collapse = "/") @@ -181,6 +179,8 @@ expect_snapshot_file <- function( comp <- NULL } + hint <- snapshot_hint(paste0(file, "/"), show_accept = is_text) + msg <- c( sprintf("Snapshot of %s has changed.", lab), comp, @@ -207,28 +207,6 @@ announce_snapshot_file <- function(path, name = basename(path)) { } } -snapshot_review_hint <- function( - test, - name, - is_text = FALSE, - reset_output = TRUE -) { - if (reset_output) { - local_reporter_output() - } - - c( - if (is_text) { - cli::format_inline( - "* Run {.run testthat::snapshot_accept('{test}/{name}')} to accept the change." - ) - }, - cli::format_inline( - "* Run {.run testthat::snapshot_review('{test}/{name}')} to review the change." - ) - ) -} - snapshot_file_equal <- function( snap_dir, # _snaps/ snap_test, # test file name diff --git a/R/snapshot.R b/R/snapshot.R index 952673bcf..16f6bcb09 100644 --- a/R/snapshot.R +++ b/R/snapshot.R @@ -367,18 +367,12 @@ expect_snapshot_helper <- function( } else { variant_lab <- "" } - if (in_check_reporter()) { - hint <- "" - } else { - hint <- snapshot_accept_hint(variant, snapshotter$file) - } if (length(comp) != 0) { - msg <- sprintf( - "Snapshot of %s has changed%s:\n%s\n\n%s", - lab, - variant_lab, - paste0(comp, collapse = "\n\n"), + hint <- snapshot_hint(snapshotter$file) + msg <- c( + sprintf("Snapshot of %s has changed%s:", lab, variant_lab), + comp, hint ) return(snapshot_fail(msg, trace_env = trace_env)) @@ -387,28 +381,59 @@ expect_snapshot_helper <- function( pass(NULL) } -snapshot_accept_hint <- function(variant, file, reset_output = TRUE) { +snapshot_hint <- function(id, show_accept = TRUE, reset_output = TRUE) { + if (in_check_reporter()) { + return("") + } + if (reset_output) { local_reporter_output() } - if (is.null(variant) || variant == "_default") { - name <- file + full_name <- paste0(id, collapse = "/") + args <- c(full_name, snapshot_hint_path()) + args <- encodeString(args, quote = '"') + args <- paste0(args, collapse = ", ") + + accept_link <- cli::format_inline("{.run testthat::snapshot_accept({args})}") + review_link <- cli::format_inline("{.run testthat::snapshot_review({args})}") + + out <- c( + if (show_accept) sprintf("* Run %s to accept the change.", accept_link), + sprintf("* Run %s to review the change.", review_link) + ) + structure(out, class = "testthat_hint") +} + +# Include path argument if we're in a different working directory +snapshot_hint_path <- function() { + wd <- Sys.getenv("TESTTHAT_WD", unset = "") + if (wd == "") { + return() + } + + test_path <- file.path(wd, "tests/testthat") + if (test_path == getwd()) { + return() + } + + old <- normalizePath(wd) + new <- normalizePath(getwd()) + + if (startsWith(new, old)) { + substr(new, nchar(old) + 2, nchar(new)) } else { - name <- file.path(variant, file) + new } +} - paste0( - cli::format_inline( - "* Run {.run testthat::snapshot_accept('{name}')} to accept the change." - ), - "\n", - cli::format_inline( - "* Run {.run testthat::snapshot_review('{name}')} to review the change." - ) - ) +#' @export +print.testthat_hint <- function(x, ...) { + cat(paste0(x, "\n", collapse = "")) + invisible(x) } + snapshot_not_available <- function(message) { local_reporter_output() diff --git a/tests/testthat/_snaps/snapshot-file.md b/tests/testthat/_snaps/snapshot-file.md index 522e28e44..10e62fac0 100644 --- a/tests/testthat/_snaps/snapshot-file.md +++ b/tests/testthat/_snaps/snapshot-file.md @@ -25,33 +25,30 @@ # generates informative hint Code - base::writeLines(snapshot_review_hint("lala", "foo.R", reset_output = FALSE)) + snapshot_hint("lala", reset_output = FALSE) Output - * Run `testthat::snapshot_review('lala/foo.R')` to review the change. - ---- - - Code - base::writeLines(snapshot_review_hint("lala", "foo.R", is_text = TRUE, - reset_output = FALSE)) - Output - * Run `testthat::snapshot_accept('lala/foo.R')` to accept the change. - * Run `testthat::snapshot_review('lala/foo.R')` to review the change. + * Run `testthat::snapshot_accept("lala")` to accept the change. + * Run `testthat::snapshot_review("lala")` to review the change. # expect_snapshot_file validates its inputs Code - expect_snapshot_file(123, "test.txt") + expect_snapshot_file(123) Condition Error in `expect_snapshot_file()`: ! `path` must be a single string, not the number 123. Code - expect_snapshot_file("test.txt", 123) + expect_snapshot_file("doesnt-exist.txt") + Condition + Error in `expect_snapshot_file()`: + ! 'doesnt-exist.txt' doesn't exist. + Code + expect_snapshot_file(path, 123) Condition Error in `expect_snapshot_file()`: ! `name` must be a single string, not the number 123. Code - expect_snapshot_file("test.txt", "test.txt", cran = "yes") + expect_snapshot_file(path, "test.txt", cran = "yes") Condition Error in `expect_snapshot_file()`: ! `cran` must be `TRUE` or `FALSE`, not the string "yes". diff --git a/tests/testthat/_snaps/snapshot.md b/tests/testthat/_snaps/snapshot.md index f9faacec4..0342f19a7 100644 --- a/tests/testthat/_snaps/snapshot.md +++ b/tests/testthat/_snaps/snapshot.md @@ -264,15 +264,10 @@ # hint is informative Code - cat(snapshot_accept_hint("_default", "bar.R", reset_output = FALSE)) + snapshot_hint("bar.R", reset_output = FALSE) Output - * Run `testthat::snapshot_accept('bar.R')` to accept the change. - * Run `testthat::snapshot_review('bar.R')` to review the change. - Code - cat(snapshot_accept_hint("foo", "bar.R", reset_output = FALSE)) - Output - * Run `testthat::snapshot_accept('foo/bar.R')` to accept the change. - * Run `testthat::snapshot_review('foo/bar.R')` to review the change. + * Run `testthat::snapshot_accept("bar.R")` to accept the change. + * Run `testthat::snapshot_review("bar.R")` to review the change. # expect_snapshot validates its inputs diff --git a/tests/testthat/test-snapshot-file.R b/tests/testthat/test-snapshot-file.R index f0e4d885a..5151674b2 100644 --- a/tests/testthat/test-snapshot-file.R +++ b/tests/testthat/test-snapshot-file.R @@ -163,24 +163,19 @@ test_that("split_path handles edge cases", { }) test_that("generates informative hint", { - expect_snapshot(base::writeLines(snapshot_review_hint( - "lala", - "foo.R", - reset_output = FALSE - ))) - - expect_snapshot(base::writeLines(snapshot_review_hint( - "lala", - "foo.R", - is_text = TRUE, - reset_output = FALSE - ))) + local_mocked_bindings(in_check_reporter = function() FALSE) + withr::local_envvar(GITHUB_ACTIONS = "false", TESTTHAT_WD = NA) + + expect_snapshot(snapshot_hint("lala", reset_output = FALSE)) }) test_that("expect_snapshot_file validates its inputs", { + path <- withr::local_tempfile(lines = "x") + expect_snapshot(error = TRUE, { - expect_snapshot_file(123, "test.txt") - expect_snapshot_file("test.txt", 123) - expect_snapshot_file("test.txt", "test.txt", cran = "yes") + expect_snapshot_file(123) + expect_snapshot_file("doesnt-exist.txt") + expect_snapshot_file(path, 123) + expect_snapshot_file(path, "test.txt", cran = "yes") }) }) diff --git a/tests/testthat/test-snapshot.R b/tests/testthat/test-snapshot.R index b714dfd2d..2eaac72c9 100644 --- a/tests/testthat/test-snapshot.R +++ b/tests/testthat/test-snapshot.R @@ -166,12 +166,24 @@ test_that("errors and warnings are folded", { # }) test_that("hint is informative", { - withr::local_envvar("GITHUB_ACTIONS" = "false") + local_mocked_bindings(in_check_reporter = function() FALSE) + withr::local_envvar(GITHUB_ACTIONS = "false", TESTTHAT_WD = NA) - expect_snapshot({ - cat(snapshot_accept_hint("_default", "bar.R", reset_output = FALSE)) - cat(snapshot_accept_hint("foo", "bar.R", reset_output = FALSE)) - }) + expect_snapshot(snapshot_hint("bar.R", reset_output = FALSE)) +}) + +test_that("hint includes path when WD is different", { + local_mocked_bindings(in_check_reporter = function() FALSE) + withr::local_envvar(TESTTHAT_WD = "..") + + hint <- snapshot_hint("bar.R", reset_output = FALSE) + # Can't use snapshot here because its hint will get the wrong path + expect_match( + hint, + 'snapshot_accept("bar.R", "testthat")', + fixed = TRUE, + all = FALSE + ) }) test_that("expect_snapshot requires a non-empty test label", {