From 6e9c86ccdc416d44a6a6f8172f5da0b671dcdae7 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 21 Mar 2022 17:03:00 -0400 Subject: [PATCH 1/7] Enforce that snapshot files should be unique Can not use `SnapshotReporter$snap_file_seen` as it is encouraged to use this field via `announce_snapshot_file(file)` to avoid files being deleted. However, if we record what has been saved, then no saved file should be duplicated. Once a new test file starts, `$snap_file_saved` should be reset --- R/snapshot-reporter.R | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/R/snapshot-reporter.R b/R/snapshot-reporter.R index b29d145bf..cfd596c00 100644 --- a/R/snapshot-reporter.R +++ b/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() @@ -98,6 +100,19 @@ 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) { + fail( + paste0( + "Snapshot file '", name, "' has already been saved.", + " Please provide a unique snapshot file name" + ), + trace_env = trace_env + ) + } + self$snap_file_saved <- c(self$snap_file_saved, save_path) + snapshot_file_equal( snap_test_dir = snap_dir, snap_name = name, From 220415a911b09000e8c33985798abc546c4d25f0 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 21 Mar 2022 17:04:05 -0400 Subject: [PATCH 2/7] Adjust test to be consistent to the variant value --- tests/testthat/_snaps/R4.0/snapshot-file/nickname.txt | 2 +- tests/testthat/_snaps/R4.0/snapshot.md | 4 ++-- tests/testthat/test-snapshot-file.R | 2 +- tests/testthat/test-snapshot.R | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/testthat/_snaps/R4.0/snapshot-file/nickname.txt b/tests/testthat/_snaps/R4.0/snapshot-file/nickname.txt index 5c8a85458..54df7d767 100644 --- a/tests/testthat/_snaps/R4.0/snapshot-file/nickname.txt +++ b/tests/testthat/_snaps/R4.0/snapshot-file/nickname.txt @@ -1 +1 @@ -Shake and Throw +R4.0 diff --git a/tests/testthat/_snaps/R4.0/snapshot.md b/tests/testthat/_snaps/R4.0/snapshot.md index a45c698ec..d44709750 100644 --- a/tests/testthat/_snaps/R4.0/snapshot.md +++ b/tests/testthat/_snaps/R4.0/snapshot.md @@ -1,7 +1,7 @@ # can access nickname Code - version$nickname + r_version() Output - [1] "Shake and Throw" + [1] "R4.0" diff --git a/tests/testthat/test-snapshot-file.R b/tests/testthat/test-snapshot-file.R index 851424362..4bbb7f769 100644 --- a/tests/testthat/test-snapshot-file.R +++ b/tests/testthat/test-snapshot-file.R @@ -48,7 +48,7 @@ test_that("expect_snapshot_file works in a different directory", { test_that("expect_snapshot_file works with variant", { expect_snapshot_file( - write_tmp_lines(version$nickname), + write_tmp_lines(r_version()), "nickname.txt", compare = compare_file_text, variant = r_version() diff --git a/tests/testthat/test-snapshot.R b/tests/testthat/test-snapshot.R index c3ee3408e..c8cb9f8c7 100644 --- a/tests/testthat/test-snapshot.R +++ b/tests/testthat/test-snapshot.R @@ -1,5 +1,5 @@ test_that("can access nickname", { - expect_snapshot(version$nickname, variant = r_version()) + expect_snapshot(r_version(), variant = r_version()) }) test_that("can snapshot output", { From 6bc461474bf5ca949c3e61420ff958b823aee499 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 21 Mar 2022 17:04:25 -0400 Subject: [PATCH 3/7] Move duplicated snapshot file --- .../_snaps/snapshot-file/foo-binary.csv | 33 +++++++++++++++++++ tests/testthat/test-snapshot-file.R | 2 +- 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 tests/testthat/_snaps/snapshot-file/foo-binary.csv diff --git a/tests/testthat/_snaps/snapshot-file/foo-binary.csv b/tests/testthat/_snaps/snapshot-file/foo-binary.csv new file mode 100644 index 000000000..a22b9c21e --- /dev/null +++ b/tests/testthat/_snaps/snapshot-file/foo-binary.csv @@ -0,0 +1,33 @@ +"","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 diff --git a/tests/testthat/test-snapshot-file.R b/tests/testthat/test-snapshot-file.R index 4bbb7f769..42a4cd13c 100644 --- a/tests/testthat/test-snapshot-file.R +++ b/tests/testthat/test-snapshot-file.R @@ -26,7 +26,7 @@ test_that("expect_snapshot_file works", { withr::local_options(lifecycle_verbosity = "quiet") expect_snapshot_file( path, - "foo.csv", + "foo-binary.csv", binary = FALSE ) }) From 3e53009fbfe76ac1022fb9d480e38ac167a566b8 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 21 Mar 2022 17:04:48 -0400 Subject: [PATCH 4/7] Test for error while saving a duplicated snapshot file --- tests/testthat/test-snapshot-file.R | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/testthat/test-snapshot-file.R b/tests/testthat/test-snapshot-file.R index 42a4cd13c..8a0dec3ae 100644 --- a/tests/testthat/test-snapshot-file.R +++ b/tests/testthat/test-snapshot-file.R @@ -55,6 +55,19 @@ 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()), + "nickname.txt", + compare = compare_file_text, + variant = r_version() + ), + "provide a unique snapshot file name" + ) +}) + test_that("basic workflow", { snapper <- local_snapshotter() From e7be9d9daa9cb8ecfc51b67528fc5f83039da456 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Mon, 21 Mar 2022 17:27:41 -0400 Subject: [PATCH 5/7] update r version 4.1 snaps --- tests/testthat/_snaps/R4.1/snapshot-file/nickname.txt | 2 +- tests/testthat/_snaps/R4.1/snapshot.md | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/testthat/_snaps/R4.1/snapshot-file/nickname.txt b/tests/testthat/_snaps/R4.1/snapshot-file/nickname.txt index b84b017a1..e1788856d 100644 --- a/tests/testthat/_snaps/R4.1/snapshot-file/nickname.txt +++ b/tests/testthat/_snaps/R4.1/snapshot-file/nickname.txt @@ -1 +1 @@ -Bird Hippie +R4.1 diff --git a/tests/testthat/_snaps/R4.1/snapshot.md b/tests/testthat/_snaps/R4.1/snapshot.md index d53bd60fb..6ae84994e 100644 --- a/tests/testthat/_snaps/R4.1/snapshot.md +++ b/tests/testthat/_snaps/R4.1/snapshot.md @@ -1,7 +1,6 @@ # can access nickname Code - version$nickname + r_version() Output - [1] "Bird Hippie" - + [1] "R4.1" From d7212a06e1fb11dd99ec8e6fd15014b42ec04e10 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 22 Mar 2022 10:01:11 -0400 Subject: [PATCH 6/7] Add news entry --- NEWS.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.md b/NEWS.md index cff4878f9..5216f6c1a 100644 --- a/NEWS.md +++ b/NEWS.md @@ -23,6 +23,8 @@ * Test results show hyperlinks to failed expectation when supported (#1544). +* `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.1.2 From 34e54b2a38073cd2a3cc74a3c24deb7edc3240d7 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 22 Mar 2022 10:04:32 -0400 Subject: [PATCH 7/7] Update snapshot.md --- tests/testthat/_snaps/R4.1/snapshot.md | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/testthat/_snaps/R4.1/snapshot.md b/tests/testthat/_snaps/R4.1/snapshot.md index 6ae84994e..cb65fdc6d 100644 --- a/tests/testthat/_snaps/R4.1/snapshot.md +++ b/tests/testthat/_snaps/R4.1/snapshot.md @@ -4,3 +4,4 @@ r_version() Output [1] "R4.1" +