Enforce uniqueness of snapshot files (#1592)

And slightly improve error messages/trace frame.

Co-authored-by: Hadley Wickham <[email protected]>

Author: schloerke

NEWS.md

@@ -1,5 +1,6 @@
 # testthat (development version)
 
+* `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)
 * `test_dir()`, `test_file()`, `test_package()`, `test_check()`, `test_local()`, `source_file()` gain a `shuffle` argument uses `sample()` to randomly reorder the top-level expressions in each test file (#1942). This random reordering surfaces dependencies between tests and code outside of any test, as well as dependencies between tests. This helps you find and eliminate unintentional dependencies.
 * `snapshot_accept(test)` now works when the test file name contains `.` (#1669).
 * `local_mock()` and `with_mock()` have been deprecated because they are no longer permitted in R 4.5.

R/snapshot-file.R

@@ -137,7 +137,6 @@ expect_snapshot_file <- function(
     path,
     file_equal = compare,
     variant = variant,
-    trace_env = caller_env()
   )
   if (inherits(equal, "expectation_failure")) {
     return(equal)
@@ -204,7 +203,7 @@ snapshot_file_equal <- function(
   trace_env = caller_env()
 ) {
   if (!file.exists(path)) {
-    cli::cli_abort("{.path {path}} not found.")
+    cli::cli_abort("{.path {path}} not found.", call = trace_env)
   }
 
   if (is.null(snap_variant)) {

R/snapshot-reporter.R

@@ -7,6 +7,7 @@ SnapshotReporter <- R6::R6Class(
     test = NULL,
     test_file_seen = character(),
     snap_file_seen = character(),
+    snap_file_saved = character(),
     variants_changed = FALSE,
     fail_on_new = NULL,
 
@@ -22,6 +23,7 @@ SnapshotReporter <- R6::R6Class(
     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()
 
@@ -107,6 +109,15 @@ SnapshotReporter <- R6::R6Class(
     ) {
       self$announce_file_snapshot(name)
 
+      save_path <- paste0(c(self$file, variant, name), collapse = "/")
+      if (save_path %in% self$snap_file_saved) {
+        cli::cli_abort(
+          "Snapshot file names must be unique. {.arg name} has already been used.",
+          call = trace_env
+        )
+      }
+      self$snap_file_saved <- c(self$snap_file_saved, save_path)
+
       snapshot_file_equal(
         snap_dir = self$snap_dir,
         snap_test = self$file,

tests/testthat/_snaps/snapshot-file.md

@@ -1,3 +1,11 @@
+# expect_snapshot_file finds duplicate snapshot files
+
+    Code
+      expect_snapshot_file(write_tmp_lines(r_version()), "version.txt", variant = r_version())
+    Condition
+      Error in `expect_snapshot_file()`:
+      ! Snapshot file names must be unique. `name` has already been used.
+
 # warns on first creation
 
     Code
@@ -11,7 +19,7 @@
     Code
       snapshot_file_equal_("doesnt-exist.txt")
     Condition
-      Error in `snapshot_file_equal()`:
+      Error in `snapshot_file_equal_()`:
       ! 'doesnt-exist.txt' not found.
 
 # snapshot_hint output differs in R CMD check

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

@@ -9,14 +9,13 @@ 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, "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-not-binary.csv", binary = FALSE)
 })
 
 
@@ -39,6 +38,17 @@ test_that("expect_snapshot_file works with variant", {
   )
 })
 
+test_that("expect_snapshot_file finds duplicate snapshot files", {
+  expect_snapshot(
+    expect_snapshot_file(
+      write_tmp_lines(r_version()),
+      "version.txt",
+      variant = r_version()
+    ),
+    error = TRUE
+  )
+})
+
 test_that("basic workflow", {
   snapper <- local_snapshotter(fail_on_new = FALSE)