diff --git a/NEWS.md b/NEWS.md index b70a51374..c1c21a602 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). * The failure messages for all `expect_` functions have been rewritten to first state what was expected and then what was actually received (#2142). * `test_file(desc = ...)` no longer loses snapshot results (#2066). * In `R CMD check`, snapshots now only advise on how to resolve failures once (#2207). 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) } diff --git a/R/snapshot-file.R b/R/snapshot-file.R index edc25e1bd..4da38f96b 100644 --- a/R/snapshot-file.R +++ b/R/snapshot-file.R @@ -106,6 +106,8 @@ expect_snapshot_file <- function( transform = NULL, variant = NULL ) { + lab <- quo_label(enquo(path)) + check_string(path) check_string(name) check_bool(cran) @@ -125,6 +127,7 @@ expect_snapshot_file <- function( return(invisible()) } + is_text <- is_text_file(name) if (!is_missing(binary)) { lifecycle::deprecate_soft( "3.0.3", @@ -134,8 +137,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 } @@ -145,7 +146,6 @@ expect_snapshot_file <- function( brio::write_lines(lines, path) } - lab <- quo_label(enquo(path)) equal <- snapshotter$take_file_snapshot( name, path, @@ -156,17 +156,34 @@ expect_snapshot_file <- function( return(equal) } + file <- snapshotter$file if (in_check_reporter()) { hint <- "" } else { - hint <- snapshot_review_hint(snapshotter$file, name) + 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(snapshot_fail(msg)) @@ -174,6 +191,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)) { @@ -185,13 +207,25 @@ announce_snapshot_file <- function(path, name = basename(path)) { } } -snapshot_review_hint <- function(test, name, reset_output = TRUE) { +snapshot_review_hint <- function( + test, + name, + is_text = FALSE, + reset_output = TRUE +) { if (reset_output) { local_reporter_output() } - cli::format_inline( - "* Run {.run testthat::snapshot_review('{test}/{name}')} to review the change." + c( + if (is_text) { + cli::format_inline( + "* Run {.run testthat::snapshot_accept('{test}/{name}')} to accept the change." + ) + }, + cli::format_inline( + "* 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 ea65d1146..522e28e44 100644 --- a/tests/testthat/_snaps/snapshot-file.md +++ b/tests/testthat/_snaps/snapshot-file.md @@ -25,9 +25,18 @@ # generates informative hint Code - cat(snapshot_review_hint("lala", "foo.r", reset_output = FALSE)) + base::writeLines(snapshot_review_hint("lala", "foo.R", reset_output = FALSE)) Output - * Run `testthat::snapshot_review('lala/foo.r')` to review the change. + * Run `testthat::snapshot_review('lala/foo.R')` to review the change. + +--- + + Code + base::writeLines(snapshot_review_hint("lala", "foo.R", is_text = TRUE, + reset_output = FALSE)) + Output + * 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 493f9be39..f0e4d885a 100644 --- a/tests/testthat/test-snapshot-file.R +++ b/tests/testthat/test-snapshot-file.R @@ -163,9 +163,16 @@ 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", + "foo.R", + reset_output = FALSE + ))) + + expect_snapshot(base::writeLines(snapshot_review_hint( + "lala", + "foo.R", + is_text = TRUE, reset_output = FALSE ))) })