From c4bdd9b9c362357ad49a7a1400cf1ffc8b17a949 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 12 Aug 2025 08:11:35 -0500 Subject: [PATCH 1/5] Display diffs when snapshotting text files I haven't yet figured out how to test snapshots with snapshots, so you can run this to test interactively ```R test_that("expect_snapshot_file() show differences for text files", { path1 <- withr::local_tempfile(lines = sample(letters[1:10])) expect_snapshot_file(path1, "text-diff.txt") }) ``` Fixes #1593 --- NEWS.md | 2 ++ R/snapshot-file.R | 47 ++++++++++++++++++++------ tests/testthat/_snaps/snapshot-file.md | 2 +- 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/NEWS.md b/NEWS.md index c3f30caab..e0e225f08 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,7 @@ # testthat (development version) +* `expect_snapshot_file()` now considers `.json` to be a text file (#1593). +* `expect_snapshot_file()` now shows differences for text files (#1593). * `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). diff --git a/R/snapshot-file.R b/R/snapshot-file.R index ece60bd66..b7cd55d88 100644 --- a/R/snapshot-file.R +++ b/R/snapshot-file.R @@ -94,6 +94,8 @@ expect_snapshot_file <- function( transform = NULL, variant = NULL ) { + lab <- quo_label(enquo(path)) + check_string(path) check_string(name) check_bool(cran) @@ -111,6 +113,7 @@ expect_snapshot_file <- function( return(invisible()) } + is_text <- is_text_file(name) if (!is_missing(binary)) { lifecycle::deprecate_soft( "3.0.3", @@ -120,8 +123,6 @@ expect_snapshot_file <- function( compare <- if (binary) compare_file_binary else compare_file_text } if (is.null(compare)) { - ext <- tools::file_ext(name) - is_text <- ext %in% c("r", "R", "txt", "md", "Rmd") compare <- if (is_text) compare_file_text else compare_file_binary } @@ -131,7 +132,6 @@ expect_snapshot_file <- function( brio::write_lines(lines, path) } - lab <- quo_label(enquo(path)) equal <- snapshotter$take_file_snapshot( name, path, @@ -142,13 +142,30 @@ expect_snapshot_file <- function( return(equal) } - hint <- snapshot_review_hint(snapshotter$file, name) + file <- snapshotter$file + hint <- snapshot_review_hint(file, name, is_text = is_text) if (!equal) { - msg <- sprintf( - "Snapshot of %s to '%s' has changed\n%s", - lab, - paste0(snapshotter$file, "/", name), + if (is_text) { + base <- paste0(c(snapshotter$snap_dir, file, variant), collapse = "/") + old_path <- paste0(c(base, name), collapse = "/") + new_path <- paste0(c(base, new_name(name)), collapse = "/") + + comp <- waldo_compare( + x = brio::read_lines(old_path), + x_arg = "old", + y = brio::read_lines(new_path), + y_arg = "new", + quote_strings = FALSE + ) + comp <- c("Differences:", comp) + } else { + comp <- NULL + } + + msg <- c( + sprintf("Snapshot of %s has changed.", lab), + comp, hint ) return(fail(msg)) @@ -156,6 +173,11 @@ expect_snapshot_file <- function( pass(NULL) } +is_text_file <- function(path) { + ext <- tools::file_ext(path) + ext %in% c("r", "R", "txt", "md", "Rmd", "qmd", "json") +} + #' @rdname expect_snapshot_file #' @export announce_snapshot_file <- function(path, name = basename(path)) { @@ -172,6 +194,7 @@ snapshot_review_hint <- function( name, ci = on_ci(), check = in_rcmd_check(), + is_text = FALSE, reset_output = TRUE ) { if (reset_output) { @@ -184,9 +207,13 @@ snapshot_review_hint <- function( if (check && ci) "* Download and unzip run artifact\n", if (check && !ci) "* Locate check directory\n", if (check) paste0("* Copy '", path, "' to local test directory\n"), - if (check) "* ", + if (is_text) { + cli::format_inline( + "* Run {.run testthat::snapshot_accept('{test}/')} to accept the change.\n" + ) + }, cli::format_inline( - "Run {.run testthat::snapshot_review('{test}/')} to review changes" + "* Run {.run testthat::snapshot_review('{test}/')} to review changes\n" ) ) } diff --git a/tests/testthat/_snaps/snapshot-file.md b/tests/testthat/_snaps/snapshot-file.md index 8d8e7b406..51272724a 100644 --- a/tests/testthat/_snaps/snapshot-file.md +++ b/tests/testthat/_snaps/snapshot-file.md @@ -27,7 +27,7 @@ Code cat(snapshot_review_hint("lala", "foo.r", check = FALSE, ci = FALSE)) Output - Run `testthat::snapshot_review('lala/')` to review changes + * Run `testthat::snapshot_review('lala/')` to review changes --- From 1380480d1cc089e76434c2244edc8ee3e66c9884 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 27 Aug 2025 15:09:38 -0500 Subject: [PATCH 2/5] Elimiante no longer used argument --- R/snapshot-file.R | 7 +-- tests/testthat/_snaps/snapshot-file.md | 10 +++- tests/testthat/_snaps/snapshot-file.new.md | 57 ++++++++++++++++++++++ tests/testthat/test-snapshot-file.R | 9 +++- 4 files changed, 75 insertions(+), 8 deletions(-) create mode 100644 tests/testthat/_snaps/snapshot-file.new.md diff --git a/R/snapshot-file.R b/R/snapshot-file.R index 0dd3be701..81af74d9d 100644 --- a/R/snapshot-file.R +++ b/R/snapshot-file.R @@ -160,7 +160,7 @@ expect_snapshot_file <- function( if (in_check_reporter()) { hint <- "" } else { - hint <- snapshot_review_hint(file, name, is_text = is_text) + hint <- snapshot_review_hint(file, is_text = is_text) } if (!equal) { @@ -209,7 +209,6 @@ announce_snapshot_file <- function(path, name = basename(path)) { snapshot_review_hint <- function( test, - name, is_text = FALSE, reset_output = TRUE ) { @@ -217,8 +216,6 @@ snapshot_review_hint <- function( local_reporter_output() } - path <- paste0("tests/testthat/_snaps/", test, "/", new_name(name)) - c( if (is_text) { cli::format_inline( @@ -226,7 +223,7 @@ snapshot_review_hint <- function( ) }, cli::format_inline( - "* Run {.run testthat::snapshot_review('{test}/')} to review changes." + "* Run {.run testthat::snapshot_review('{test}/')} to review the change." ) ) } diff --git a/tests/testthat/_snaps/snapshot-file.md b/tests/testthat/_snaps/snapshot-file.md index 1f1d06cbe..13c89dbe5 100644 --- a/tests/testthat/_snaps/snapshot-file.md +++ b/tests/testthat/_snaps/snapshot-file.md @@ -25,10 +25,18 @@ # generates informative hint Code - cat(snapshot_review_hint("lala", "foo.r", reset_output = FALSE)) + base::writeLines(snapshot_review_hint("lala", reset_output = FALSE)) Output * Run `testthat::snapshot_review('lala/')` to review changes. +--- + + Code + base::writeLines(snapshot_review_hint("lala", is_text = TRUE, reset_output = FALSE)) + Output + * Run `testthat::snapshot_accept('lala/')` to accept the change. + * Run `testthat::snapshot_review('lala/')` to review changes. + # expect_snapshot_file validates its inputs Code diff --git a/tests/testthat/_snaps/snapshot-file.new.md b/tests/testthat/_snaps/snapshot-file.new.md new file mode 100644 index 000000000..c3a5317bc --- /dev/null +++ b/tests/testthat/_snaps/snapshot-file.new.md @@ -0,0 +1,57 @@ +# 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 + out <- snapshot_file_equal_(path) + Condition + Warning: + Adding new file snapshot: 'tests/testthat/_snaps/my-test/test.txt' + +--- + + Code + snapshot_file_equal_("doesnt-exist.txt") + Condition + Error in `snapshot_file_equal_()`: + ! 'doesnt-exist.txt' not found. + +# generates informative hint + + Code + base::writeLines(snapshot_review_hint("lala", reset_output = FALSE)) + Output + * Run `testthat::snapshot_review('lala/')` to review the change. + +--- + + Code + base::writeLines(snapshot_review_hint("lala", is_text = TRUE, reset_output = FALSE)) + Output + * 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") + Condition + Error in `expect_snapshot_file()`: + ! `path` must be a single string, not the number 123. + Code + expect_snapshot_file("test.txt", 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") + Condition + Error in `expect_snapshot_file()`: + ! `cran` must be `TRUE` or `FALSE`, not the string "yes". + diff --git a/tests/testthat/test-snapshot-file.R b/tests/testthat/test-snapshot-file.R index 493f9be39..139366e73 100644 --- a/tests/testthat/test-snapshot-file.R +++ b/tests/testthat/test-snapshot-file.R @@ -163,9 +163,14 @@ test_that("split_path handles edge cases", { }) test_that("generates informative hint", { - expect_snapshot(cat(snapshot_review_hint( + expect_snapshot(base::writeLines(snapshot_review_hint( "lala", - "foo.r", + reset_output = FALSE + ))) + + expect_snapshot(base::writeLines(snapshot_review_hint( + "lala", + is_text = TRUE, reset_output = FALSE ))) }) From 98158712855f73e2bb9af8d670fc6ab3ebe323ed Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 27 Aug 2025 15:10:40 -0500 Subject: [PATCH 3/5] Update snapshot --- tests/testthat/_snaps/snapshot-file.md | 4 +- tests/testthat/_snaps/snapshot-file.new.md | 57 ---------------------- 2 files changed, 2 insertions(+), 59 deletions(-) delete mode 100644 tests/testthat/_snaps/snapshot-file.new.md diff --git a/tests/testthat/_snaps/snapshot-file.md b/tests/testthat/_snaps/snapshot-file.md index 13c89dbe5..c3a5317bc 100644 --- a/tests/testthat/_snaps/snapshot-file.md +++ b/tests/testthat/_snaps/snapshot-file.md @@ -27,7 +27,7 @@ Code base::writeLines(snapshot_review_hint("lala", reset_output = FALSE)) Output - * Run `testthat::snapshot_review('lala/')` to review changes. + * Run `testthat::snapshot_review('lala/')` to review the change. --- @@ -35,7 +35,7 @@ base::writeLines(snapshot_review_hint("lala", is_text = TRUE, reset_output = FALSE)) Output * Run `testthat::snapshot_accept('lala/')` to accept the change. - * Run `testthat::snapshot_review('lala/')` to review changes. + * Run `testthat::snapshot_review('lala/')` to review the change. # expect_snapshot_file validates its inputs diff --git a/tests/testthat/_snaps/snapshot-file.new.md b/tests/testthat/_snaps/snapshot-file.new.md deleted file mode 100644 index c3a5317bc..000000000 --- a/tests/testthat/_snaps/snapshot-file.new.md +++ /dev/null @@ -1,57 +0,0 @@ -# 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 - out <- snapshot_file_equal_(path) - Condition - Warning: - Adding new file snapshot: 'tests/testthat/_snaps/my-test/test.txt' - ---- - - Code - snapshot_file_equal_("doesnt-exist.txt") - Condition - Error in `snapshot_file_equal_()`: - ! 'doesnt-exist.txt' not found. - -# generates informative hint - - Code - base::writeLines(snapshot_review_hint("lala", reset_output = FALSE)) - Output - * Run `testthat::snapshot_review('lala/')` to review the change. - ---- - - Code - base::writeLines(snapshot_review_hint("lala", is_text = TRUE, reset_output = FALSE)) - Output - * 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") - Condition - Error in `expect_snapshot_file()`: - ! `path` must be a single string, not the number 123. - Code - expect_snapshot_file("test.txt", 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") - Condition - Error in `expect_snapshot_file()`: - ! `cran` must be `TRUE` or `FALSE`, not the string "yes". - From 6006881c02490070e6cf779a08a8068f83a9deb1 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 27 Aug 2025 15:12:30 -0500 Subject: [PATCH 4/5] Fix snapshot failure difference upstream --- R/expect-self-test.R | 2 +- R/expect-that.R | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/R/expect-self-test.R b/R/expect-self-test.R index 56c7cde53..05da3bf9f 100644 --- a/R/expect-self-test.R +++ b/R/expect-self-test.R @@ -88,7 +88,7 @@ expect_failure <- function(expr, message = NULL, ...) { if (!is.null(message)) { act <- labelled_value(status$last_failure$message, "failure message") - return(expect_match_(act, message, ..., title = "message", all = FALSE)) + return(expect_match_(act, message, ..., title = "message")) } pass(NULL) } diff --git a/R/expect-that.R b/R/expect-that.R index b32cd7098..9579d6f1c 100644 --- a/R/expect-that.R +++ b/R/expect-that.R @@ -48,6 +48,7 @@ fail <- function( snapshot_fail <- function(message, trace_env = caller_env()) { trace <- capture_trace(trace_env) + message <- paste(message, collapse = "\n") expectation("failure", message, trace = trace, snapshot = TRUE) } From 50cf4198234b0e41e2e242a2ff6f6252ce6af519 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 27 Aug 2025 15:15:12 -0500 Subject: [PATCH 5/5] Oops it was used --- R/snapshot-file.R | 7 ++++--- tests/testthat/_snaps/snapshot-file.md | 11 ++++++----- tests/testthat/test-snapshot-file.R | 2 ++ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/R/snapshot-file.R b/R/snapshot-file.R index 81af74d9d..4da38f96b 100644 --- a/R/snapshot-file.R +++ b/R/snapshot-file.R @@ -160,7 +160,7 @@ expect_snapshot_file <- function( if (in_check_reporter()) { hint <- "" } else { - hint <- snapshot_review_hint(file, is_text = is_text) + hint <- snapshot_review_hint(file, name, is_text = is_text) } if (!equal) { @@ -209,6 +209,7 @@ announce_snapshot_file <- function(path, name = basename(path)) { snapshot_review_hint <- function( test, + name, is_text = FALSE, reset_output = TRUE ) { @@ -219,11 +220,11 @@ snapshot_review_hint <- function( c( if (is_text) { cli::format_inline( - "* Run {.run testthat::snapshot_accept('{test}/')} to accept the change." + "* Run {.run testthat::snapshot_accept('{test}/{name}')} to accept the change." ) }, cli::format_inline( - "* Run {.run testthat::snapshot_review('{test}/')} to review the change." + "* Run {.run testthat::snapshot_review('{test}/{name}')} to review the change." ) ) } diff --git a/tests/testthat/_snaps/snapshot-file.md b/tests/testthat/_snaps/snapshot-file.md index c3a5317bc..522e28e44 100644 --- a/tests/testthat/_snaps/snapshot-file.md +++ b/tests/testthat/_snaps/snapshot-file.md @@ -25,17 +25,18 @@ # generates informative hint Code - base::writeLines(snapshot_review_hint("lala", reset_output = FALSE)) + base::writeLines(snapshot_review_hint("lala", "foo.R", reset_output = FALSE)) Output - * Run `testthat::snapshot_review('lala/')` to review the change. + * Run `testthat::snapshot_review('lala/foo.R')` to review the change. --- Code - base::writeLines(snapshot_review_hint("lala", is_text = TRUE, reset_output = FALSE)) + base::writeLines(snapshot_review_hint("lala", "foo.R", is_text = TRUE, + reset_output = FALSE)) Output - * Run `testthat::snapshot_accept('lala/')` to accept the change. - * Run `testthat::snapshot_review('lala/')` to review the change. + * Run `testthat::snapshot_accept('lala/foo.R')` to accept the change. + * Run `testthat::snapshot_review('lala/foo.R')` to review the change. # expect_snapshot_file validates its inputs diff --git a/tests/testthat/test-snapshot-file.R b/tests/testthat/test-snapshot-file.R index 139366e73..f0e4d885a 100644 --- a/tests/testthat/test-snapshot-file.R +++ b/tests/testthat/test-snapshot-file.R @@ -165,11 +165,13 @@ 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 )))