Include test path in snaphot hints (#2191)

If you're not testing the current directory/package.

(Also includes a path bug fix and an improved error if the `path` doesn't exist)

Fixes #1577

Author: hadley

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)

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).

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
   )
 }

R/snapshot-file.R

@@ -114,6 +114,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)
@@ -162,15 +165,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 = "/")
 
@@ -186,6 +184,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,
@@ -212,28 +212,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

R/snapshot.R

@@ -372,18 +372,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))
@@ -392,28 +386,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()
 

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".

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
 

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")
   })
 })

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", {