Enforce that snapshot files should be unique

NEWS.md

@@ -8,6 +8,8 @@
 * New `skip_unless_r()` to skip running tests on unsuitable versions of R, e.g. `skip_unless_r(">= 4.1.0")` to skip tests that require, say, `...names` (@MichaelChirico, #2022)
 * `skip_on_os()` gains an option `"emscripten"` of the `os` argument to skip tests on Emscripten (@eitsupi, #2103).
 * New expectation, `expect_shape()`, for testing the shape (i.e., the `length()`, `nrow()`, `ncol()`, or `dim()`), all in one place (#1423, @michaelchirico).
+* `expect_snapshot_file(name=)` must have a unique file path. If a snapshot file attempts to be saved with a duplicate `name`, an error will be thrown. (#1592)
+
 
 # testthat 3.2.3
 
@@ -298,7 +300,6 @@
 
 * Test results show hyperlinks to failed expectation when supported (#1544).
 
-
 # testthat 3.1.2
 
 * testthat now uses brio for all reading and writing (#1120). This

R/snapshot-reporter.R

@@ -7,6 +7,7 @@ SnapshotReporter <- R6::R6Class("SnapshotReporter",
     test = NULL,
     test_file_seen = character(),
     snap_file_seen = character(),
+    snap_file_saved = character(),
     variants_changed = FALSE,
     fail_on_new = FALSE,
 
@@ -22,6 +23,7 @@ SnapshotReporter <- R6::R6Class("SnapshotReporter",
     start_file = function(path, test = NULL) {
       self$file <- context_name(path)
       self$test_file_seen <- c(self$test_file_seen, self$file)
+      self$snap_file_saved <- character()
 
       self$variants_changed <- character()
 
@@ -101,6 +103,15 @@ SnapshotReporter <- R6::R6Class("SnapshotReporter",
       } else {
         snap_dir <- file.path(self$snap_dir, variant, self$file)
       }
+
+      save_path <- file.path(snap_dir, name)
+      if (save_path %in% self$snap_file_saved) {
+        cli::cli_abort(
+          "Snapshot file {.arg name} has already been saved. Please provide a unique snapshot file name."
+        )
+      }
+      self$snap_file_saved <- c(self$snap_file_saved, save_path)
+
       snapshot_file_equal(
         snap_test_dir = snap_dir,
         snap_name = name,

tests/testthat/_snaps/snapshot-file/foo-not-binary.csv

@@ -0,0 +1,6 @@
+"","mpg","cyl","disp","hp"
+"Mazda RX4",21,6,160,110
+"Mazda RX4 Wag",21,6,160,110
+"Datsun 710",22.8,4,108,93
+"Hornet 4 Drive",21.4,6,258,110
+"Hornet Sportabout",18.7,8,360,175

tests/testthat/_snaps/snapshot-file/foo.csv

@@ -1,33 +1,6 @@
-"","mpg","cyl","disp","hp","drat","wt","qsec","vs","am","gear","carb"
-"Mazda RX4",21,6,160,110,3.9,2.62,16.46,0,1,4,4
-"Mazda RX4 Wag",21,6,160,110,3.9,2.875,17.02,0,1,4,4
-"Datsun 710",22.8,4,108,93,3.85,2.32,18.61,1,1,4,1
-"Hornet 4 Drive",21.4,6,258,110,3.08,3.215,19.44,1,0,3,1
-"Hornet Sportabout",18.7,8,360,175,3.15,3.44,17.02,0,0,3,2
-"Valiant",18.1,6,225,105,2.76,3.46,20.22,1,0,3,1
-"Duster 360",14.3,8,360,245,3.21,3.57,15.84,0,0,3,4
-"Merc 240D",24.4,4,146.7,62,3.69,3.19,20,1,0,4,2
-"Merc 230",22.8,4,140.8,95,3.92,3.15,22.9,1,0,4,2
-"Merc 280",19.2,6,167.6,123,3.92,3.44,18.3,1,0,4,4
-"Merc 280C",17.8,6,167.6,123,3.92,3.44,18.9,1,0,4,4
-"Merc 450SE",16.4,8,275.8,180,3.07,4.07,17.4,0,0,3,3
-"Merc 450SL",17.3,8,275.8,180,3.07,3.73,17.6,0,0,3,3
-"Merc 450SLC",15.2,8,275.8,180,3.07,3.78,18,0,0,3,3
-"Cadillac Fleetwood",10.4,8,472,205,2.93,5.25,17.98,0,0,3,4
-"Lincoln Continental",10.4,8,460,215,3,5.424,17.82,0,0,3,4
-"Chrysler Imperial",14.7,8,440,230,3.23,5.345,17.42,0,0,3,4
-"Fiat 128",32.4,4,78.7,66,4.08,2.2,19.47,1,1,4,1
-"Honda Civic",30.4,4,75.7,52,4.93,1.615,18.52,1,1,4,2
-"Toyota Corolla",33.9,4,71.1,65,4.22,1.835,19.9,1,1,4,1
-"Toyota Corona",21.5,4,120.1,97,3.7,2.465,20.01,1,0,3,1
-"Dodge Challenger",15.5,8,318,150,2.76,3.52,16.87,0,0,3,2
-"AMC Javelin",15.2,8,304,150,3.15,3.435,17.3,0,0,3,2
-"Camaro Z28",13.3,8,350,245,3.73,3.84,15.41,0,0,3,4
-"Pontiac Firebird",19.2,8,400,175,3.08,3.845,17.05,0,0,3,2
-"Fiat X1-9",27.3,4,79,66,4.08,1.935,18.9,1,1,4,1
-"Porsche 914-2",26,4,120.3,91,4.43,2.14,16.7,0,1,5,2
-"Lotus Europa",30.4,4,95.1,113,3.77,1.513,16.9,1,1,5,2
-"Ford Pantera L",15.8,8,351,264,4.22,3.17,14.5,0,1,5,4
-"Ferrari Dino",19.7,6,145,175,3.62,2.77,15.5,0,1,5,6
-"Maserati Bora",15,8,301,335,3.54,3.57,14.6,0,1,5,8
-"Volvo 142E",21.4,4,121,109,4.11,2.78,18.6,1,1,4,2
+"","mpg","cyl","disp","hp"
+"Mazda RX4",21,6,160,110
+"Mazda RX4 Wag",21,6,160,110
+"Datsun 710",22.8,4,108,93
+"Hornet 4 Drive",21.4,6,258,110
+"Hornet Sportabout",18.7,8,360,175

tests/testthat/test-snapshot-file.R

@@ -12,8 +12,7 @@ test_that("expect_snapshot_file works", {
   expect_snapshot_file(path, "foo.png")
 
   path <- withr::local_tempfile()
-  mtcars2 <- mtcars
-  # mtcars2$wt[10] <- NA
+  mtcars2 <- mtcars[1:5, 1:4]
   write.csv(mtcars2, path)
   expect_snapshot_file(
     path,
@@ -25,7 +24,7 @@ test_that("expect_snapshot_file works", {
   withr::local_options(lifecycle_verbosity = "quiet")
   expect_snapshot_file(
     path,
-    "foo.csv",
+    "foo-not-binary.csv",
     binary = FALSE
   )
 })
@@ -53,12 +52,28 @@ test_that("expect_snapshot_file works with variant", {
   )
 })
 
+test_that("expect_snapshot_file finds duplicate snapshot files", {
+  # Save to the same file as in previous test
+  expect_error(
+    expect_snapshot_file(
+      write_tmp_lines(r_version()),
+      "version.txt",
+      compare = compare_file_text,
+      variant = r_version()
+    ),
+    "provide a unique snapshot file name"
+  )
+})
+
 test_that("basic workflow", {
   snapper <- local_snapshotter()
 
   # 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(write_tmp_lines(letters), "letters.txt"),
+    "Adding new"
+  )
   snapper$end_file()
 
   # succeeds if unchanged
@@ -68,7 +83,10 @@ test_that("basic workflow", {
 
   # fails if changed
   snapper$start_file("snapshot-6", "test")
-  expect_failure(expect_snapshot_file(write_tmp_lines(letters[-1]), "letters.txt"))
+  expect_failure(expect_snapshot_file(
+    write_tmp_lines(letters[-1]),
+    "letters.txt"
+  ))
   snapper$end_file()
 })
 
@@ -100,11 +118,15 @@ test_that("warns on first creation", {
 
   # Errors on non-existing file
   expect_error(
-    expect_true(snapshot_file_equal(tempdir(), "test.txt", NULL, "doesnt-exist.txt")),
+    expect_true(snapshot_file_equal(
+      tempdir(),
+      "test.txt",
+      NULL,
+      "doesnt-exist.txt"
+    )),
     "`doesnt-exist.txt` not found"
   )
 
-
   # Unchanged returns TRUE
   expect_true(snapshot_file_equal(tempdir(), "test.txt", NULL, path))
   expect_true(file.exists(file.path(tempdir(), "test.txt")))
@@ -156,7 +178,22 @@ test_that("snapshot_hint output differs in R CMD check", {
     testthat:::snapshot_review_hint(..., reset_output = FALSE)
   }
 
-  expect_snapshot(cat(snapshot_review_hint("lala", "foo.r", check = FALSE, ci = FALSE)))
-  expect_snapshot(cat(snapshot_review_hint("lala", "foo.r", check = TRUE, ci = FALSE)))
-  expect_snapshot(cat(snapshot_review_hint("lala", "foo.r", check = TRUE, ci = TRUE)))
+  expect_snapshot(cat(snapshot_review_hint(
+    "lala",
+    "foo.r",
+    check = FALSE,
+    ci = FALSE
+  )))
+  expect_snapshot(cat(snapshot_review_hint(
+    "lala",
+    "foo.r",
+    check = TRUE,
+    ci = FALSE
+  )))
+  expect_snapshot(cat(snapshot_review_hint(
+    "lala",
+    "foo.r",
+    check = TRUE,
+    ci = TRUE
+  )))
 })

tests/testthat/test-snapshot.R

@@ -39,8 +39,14 @@ test_that("multiple outputs of same type are collapsed", {
   expect_snapshot({
     x <- 1
     y <- 1
-    {message("a"); message("b")}
-    {warning("a"); warning("b")}
+    {
+      message("a")
+      message("b")
+    }
+    {
+      warning("a")
+      warning("b")
+    }
   })
 })
 
@@ -169,9 +175,7 @@ test_that("hint is informative", {
 })
 
 test_that("expect_snapshot requires a non-empty test label", {
-
   test_that("", {
     expect_error(expect_snapshot(1 + 1))
   })
-
 })