Show correct path in expect_snapshot_file()

Was previously missing the test name, which is part of the path for snapshot files

Fixes #2090

Author: hadley

R/snapshot-file.R

@@ -188,11 +188,13 @@ snapshot_review_hint <- function(
   )
 }
 
+
 snapshot_file_equal <- function(
-  snap_test_dir,
-  snap_name,
-  snap_variant,
-  path,
+  snap_dir, # _snaps/
+  snap_test, # test file name
+  snap_name, # snapshot file name
+  snap_variant, # variant (optional)
+  path, # path to new file
   file_equal = compare_file_binary,
   fail_on_new = FALSE,
   trace_env = caller_env()
@@ -201,8 +203,14 @@ snapshot_file_equal <- function(
     abort(paste0("`", path, "` not found"))
   }
 
+  if (is.null(snap_variant)) {
+    snap_test_dir <- file.path(snap_dir, snap_test)
+  } else {
+    snap_test_dir <- file.path(snap_dir, snap_variant, snap_test)
+  }
+
   cur_path <- file.path(snap_test_dir, snap_name)
-  new_path <- new_name(cur_path)
+  new_path <- file.path(snap_test_dir, new_name(snap_name))
 
   if (file.exists(cur_path)) {
     eq <- file_equal(cur_path, path)
@@ -217,10 +225,10 @@ snapshot_file_equal <- function(
     dir.create(snap_test_dir, showWarnings = FALSE, recursive = TRUE)
     file.copy(path, cur_path)
 
-    message <- paste0(
+    message <- paste_c(
       "Adding new file snapshot: 'tests/testthat/_snaps/",
-      snap_variant,
-      if (!is.null(snap_variant)) "/",
+      c(snap_variant, if (!is.null(snap_variant)) "/"),
+      c(snap_test, "/"),
       snap_name,
       "'"
     )

R/snapshot-reporter.R

@@ -108,13 +108,9 @@ SnapshotReporter <- R6::R6Class(
     ) {
       self$announce_file_snapshot(name)
 
-      if (is.null(variant)) {
-        snap_dir <- file.path(self$snap_dir, self$file)
-      } else {
-        snap_dir <- file.path(self$snap_dir, variant, self$file)
-      }
       snapshot_file_equal(
-        snap_test_dir = snap_dir,
+        snap_dir = self$snap_dir,
+        snap_test = self$file,
         snap_name = name,
         snap_variant = variant,
         path = path,

R/utils.R

@@ -56,3 +56,7 @@ in_rcmd_check <- function() {
 }
 
 r_version <- function() paste0("R", getRversion()[, 1:2])
+
+paste_c <- function(...) {
+  paste0(c(...), collapse = "")
+}

tests/testthat/_snaps/snapshot-file.md

@@ -1,15 +1,15 @@
 # warns on first creation
 
     Code
-      out <- snapshot_file_equal(tempdir(), "test.txt", NULL, path)
+      out <- snapshot_file_equal_(path)
     Condition
       Warning:
-      Adding new file snapshot: 'tests/testthat/_snaps/test.txt'
+      Adding new file snapshot: 'tests/testthat/_snaps/my-test/test.txt'
 
 ---
 
     Code
-      expect_true(snapshot_file_equal(tempdir(), "test.txt", NULL, "doesnt-exist.txt"))
+      snapshot_file_equal_("doesnt-exist.txt")
     Condition
       Error in `snapshot_file_equal()`:
       ! `doesnt-exist.txt` not found

tests/testthat/test-snapshot-file.R

@@ -1,9 +1,6 @@
 test_that("expect_snapshot_file works", {
-  expect_snapshot_file(
-    write_tmp_lines(letters),
-    "foo.r",
-    compare = compare_file_text
-  )
+  path <- write_tmp_lines(letters)
+  expect_snapshot_file(path, "foo.r", compare = compare_file_text)
 
   path <- withr::local_tempfile()
   png(path, width = 300, height = 300, type = "cairo")
@@ -15,19 +12,11 @@ test_that("expect_snapshot_file works", {
   mtcars2 <- mtcars
   # mtcars2$wt[10] <- NA
   write.csv(mtcars2, path)
-  expect_snapshot_file(
-    path,
-    "foo.csv",
-    compare = compare_file_text
-  )
+  expect_snapshot_file(path, "foo.csv", compare = compare_file_text)
 
   # Deprecated `binary` argument still works
   withr::local_options(lifecycle_verbosity = "quiet")
-  expect_snapshot_file(
-    path,
-    "foo.csv",
-    binary = FALSE
-  )
+  expect_snapshot_file(path, "foo.csv", binary = FALSE)
 })
 
 
@@ -53,25 +42,21 @@ test_that("expect_snapshot_file works with variant", {
 test_that("basic workflow", {
   snapper <- local_snapshotter(fail_on_new = FALSE)
 
+  path <- write_tmp_lines(letters)
   # warns on first run
   snapper$start_file("snapshot-6", "test")
-  expect_warning(
-    expect_snapshot_file(write_tmp_lines(letters), "letters.txt"),
-    "Adding new"
-  )
+  expect_warning(expect_snapshot_file(path, "letters.txt"), "Adding new")
   snapper$end_file()
 
   # succeeds if unchanged
   snapper$start_file("snapshot-6", "test")
-  expect_success(expect_snapshot_file(write_tmp_lines(letters), "letters.txt"))
+  expect_success(expect_snapshot_file(path, "letters.txt"))
   snapper$end_file()
 
   # fails if changed
   snapper$start_file("snapshot-6", "test")
-  expect_failure(expect_snapshot_file(
-    write_tmp_lines(letters[-1]),
-    "letters.txt"
-  ))
+  path2 <- write_tmp_lines(letters[-1])
+  expect_failure(expect_snapshot_file(path2, "letters.txt"))
   snapper$end_file()
 })
 
@@ -93,42 +78,48 @@ test_that("can transform snapshot contents", {
 
 test_that("warns on first creation", {
   path <- write_tmp_lines("a")
-  withr::defer(unlink(file.path(tempdir(), "test.txt")))
+  snap_dir <- withr::local_tempdir()
+
+  snapshot_file_equal_ <- function(path) {
+    snapshot_file_equal(
+      snap_dir = snap_dir,
+      snap_test = "my-test",
+      snap_name = "test.txt",
+      snap_variant = NULL,
+      path = path
+    )
+  }
 
   # Warns on first run
-  expect_snapshot(out <- snapshot_file_equal(tempdir(), "test.txt", NULL, path))
+  expect_snapshot(out <- snapshot_file_equal_(path))
   expect_true(out)
 
   # Errors on non-existing file
-  expect_snapshot(error = TRUE, {
-    expect_true(snapshot_file_equal(
-      tempdir(),
-      "test.txt",
-      NULL,
-      "doesnt-exist.txt"
-    ))
-  })
+  expect_snapshot(snapshot_file_equal_("doesnt-exist.txt"), error = TRUE)
 
   # Unchanged returns TRUE
-  expect_true(snapshot_file_equal(tempdir(), "test.txt", NULL, path))
-  expect_true(file.exists(file.path(tempdir(), "test.txt")))
-  expect_false(file.exists(file.path(tempdir(), "test.new.txt")))
+  expect_true(snapshot_file_equal_(path))
+  expect_true(file.exists(file.path(snap_dir, "my-test/test.txt")))
+  expect_false(file.exists(file.path(snap_dir, "my-test/test.new.txt")))
 
   # Changed returns FALSE
   path2 <- write_tmp_lines("b")
-  expect_false(snapshot_file_equal(tempdir(), "test.txt", NULL, path2))
-  expect_true(file.exists(file.path(tempdir(), "test.txt")))
-  expect_true(file.exists(file.path(tempdir(), "test.new.txt")))
+  expect_false(snapshot_file_equal_(path2))
+  expect_true(file.exists(file.path(snap_dir, "my-test/test.txt")))
+  expect_true(file.exists(file.path(snap_dir, "my-test/test.new.txt")))
 
   # Changing again overwrites
   path2 <- write_tmp_lines("c")
-  expect_false(snapshot_file_equal(tempdir(), "test.txt", NULL, path2))
-  expect_equal(brio::read_lines(file.path(tempdir(), "test.new.txt")), "c")
+  expect_false(snapshot_file_equal_(path2))
+  expect_equal(
+    brio::read_lines(file.path(snap_dir, "my-test/test.new.txt")),
+    "c"
+  )
 
   # Unchanged cleans up
-  expect_true(snapshot_file_equal(tempdir(), "test.txt", NULL, path))
-  expect_true(file.exists(file.path(tempdir(), "test.txt")))
-  expect_false(file.exists(file.path(tempdir(), "test.new.txt")))
+  expect_true(snapshot_file_equal_(path))
+  expect_true(file.exists(file.path(snap_dir, "my-test/test.txt")))
+  expect_false(file.exists(file.path(snap_dir, "my-test/test.new.txt")))
 })
 
 # helpers -----------------------------------------------------------------