Actually fail with new snapshots on CI (#2187)

By defaulting `fail_on_new` to `NULL` in more places. Also involved fixing confusing errors by correctly propagating failures, and reduced minimised overall failure error message.

Continuation of #2149. Fixes #2176.

Author: hadley

R/parallel.R

@@ -83,7 +83,7 @@ test_files_parallel <- function(
 
 test_files_reporter_parallel <- function(reporter, .env = parent.frame()) {
   lister <- ListReporter$new()
-  snapshotter <- MainprocessSnapshotReporter$new("_snaps", fail_on_new = FALSE)
+  snapshotter <- MainprocessSnapshotReporter$new("_snaps")
   reporters <- list(
     find_reporter(reporter),
     lister, # track data
@@ -301,10 +301,7 @@ queue_process_setup <- function(
 
 queue_task <- function(path) {
   withr::local_envvar("TESTTHAT_IS_PARALLEL" = "true")
-  snapshotter <- SubprocessSnapshotReporter$new(
-    snap_dir = "_snaps",
-    fail_on_new = FALSE
-  )
+  snapshotter <- SubprocessSnapshotReporter$new(snap_dir = "_snaps")
   withr::local_options(testthat.snapshotter = snapshotter)
   reporters <- list(
     SubprocessReporter$new(),

R/snapshot-file.R

@@ -139,6 +139,10 @@ expect_snapshot_file <- function(
     variant = variant,
     trace_env = caller_env()
   )
+  if (inherits(equal, "expectation_failure")) {
+    return(equal)
+  }
+
   hint <- snapshot_review_hint(snapshotter$file, name)
 
   if (!equal) {
@@ -196,7 +200,7 @@ snapshot_file_equal <- function(
   snap_variant, # variant (optional)
   path, # path to new file
   file_equal = compare_file_binary,
-  fail_on_new = FALSE,
+  fail_on_new = NULL,
   trace_env = caller_env()
 ) {
   if (!file.exists(path)) {
@@ -208,6 +212,7 @@ snapshot_file_equal <- function(
   } else {
     snap_test_dir <- file.path(snap_dir, snap_variant, snap_test)
   }
+  fail_on_new <- fail_on_new %||% on_ci()
 
   cur_path <- file.path(snap_test_dir, snap_name)
   new_path <- file.path(snap_test_dir, new_name(snap_name))
@@ -232,12 +237,13 @@ snapshot_file_equal <- function(
       snap_name,
       "'"
     )
+
+    # We want to fail on CI since this suggests that the user has failed
+    # to record the value locally
     if (fail_on_new) {
       return(fail(message, trace_env = trace_env))
-    } else {
-      testthat_warn(message)
     }
-
+    testthat_warn(message)
     TRUE
   }
 }

R/snapshot-reporter.R

@@ -8,13 +8,13 @@ SnapshotReporter <- R6::R6Class(
     test_file_seen = character(),
     snap_file_seen = character(),
     variants_changed = FALSE,
-    fail_on_new = FALSE,
+    fail_on_new = NULL,
 
     old_snaps = NULL,
     cur_snaps = NULL,
     new_snaps = NULL,
 
-    initialize = function(snap_dir = "_snaps", fail_on_new = FALSE) {
+    initialize = function(snap_dir = "_snaps", fail_on_new = NULL) {
       self$snap_dir <- normalizePath(snap_dir, mustWork = FALSE)
       self$fail_on_new <- fail_on_new
     },
@@ -82,19 +82,18 @@ SnapshotReporter <- R6::R6Class(
         value_enc <- save(value)
 
         self$cur_snaps$append(self$test, variant, value_enc)
+        fail_on_new <- self$fail_on_new %||% on_ci()
 
         message <- paste0(
           "Adding new snapshot",
           if (variant != "_default") paste0(" for variant '", variant, "'"),
-          if (self$fail_on_new) " in CI",
           ":\n",
           value_enc
         )
-        if (self$fail_on_new) {
+        if (fail_on_new) {
           return(fail(message, trace_env = trace_env))
-        } else {
-          testthat_warn(message)
         }
+        testthat_warn(message)
         character()
       }
     },
@@ -196,7 +195,7 @@ get_snapshotter <- function() {
 local_snapshotter <- function(
   snap_dir = NULL,
   cleanup = FALSE,
-  fail_on_new = FALSE,
+  fail_on_new = NULL,
   .env = parent.frame()
 ) {
   snap_dir <- snap_dir %||% withr::local_tempdir(.local_envir = .env)

R/snapshot.R

@@ -331,6 +331,9 @@ expect_snapshot_helper <- function(
     variant = variant,
     trace_env = trace_env
   )
+  if (inherits(comp, "expectation_failure")) {
+    return(comp)
+  }
 
   if (!identical(variant, "_default")) {
     variant_lab <- paste0(" (variant '", variant, "')")

R/test-files.R

@@ -311,7 +311,7 @@ test_files_reporter <- function(reporter, .env = parent.frame()) {
   reporters <- list(
     find_reporter(reporter),
     lister, # track data
-    local_snapshotter("_snaps", fail_on_new = on_ci(), .env = .env)
+    local_snapshotter("_snaps", .env = .env)
   )
   list(
     multi = MultiReporter$new(reporters = compact(reporters)),
@@ -325,10 +325,18 @@ test_files_check <- function(
   stop_on_warning = FALSE
 ) {
   if (stop_on_failure && !all_passed(results)) {
-    cli::cli_abort("Test failures.")
+    cli::cli_abort(
+      "Test failures.",
+      call = NULL,
+      trace = data.frame()
+    )
   }
   if (stop_on_warning && any_warnings(results)) {
-    cli::cli_abort("Tests generated warnings.")
+    cli::cli_abort(
+      "Tests generated warnings.",
+      call = NULL,
+      trace = data.frame()
+    )
   }
 
   invisible(results)

man/local_snapshotter.Rd

@@ -0,0 +1 @@
+R4.6

tests/testthat/_snaps/R4.6/snapshot-file/version.txt

@@ -0,0 +1,7 @@
+# variants save different values
+
+    Code
+      r_version()
+    Output
+      [1] "R4.6"
+

tests/testthat/_snaps/R4.6/snapshot.md

@@ -3,7 +3,7 @@
     Code
       test_dir(test_path("test_dir"), reporter = "silent")
     Condition
-      Error in `test_files_check()`:
+      Error:
       ! Test failures.
 
 # runs all tests and records output
@@ -40,15 +40,15 @@
     Code
       test_error(stop_on_failure = TRUE)
     Condition
-      Error in `test_files_check()`:
+      Error:
       ! Test failures.
 
 # can control if warnings errors
 
     Code
       test_warning(stop_on_warning = TRUE)
     Condition
-      Error in `test_files_check()`:
+      Error:
       ! Tests generated warnings.
 
 # complains if file doesn't exist

tests/testthat/_snaps/test-files.md

@@ -60,8 +60,7 @@ test_that("fail", {
 
 test_that("snapshots", {
   withr::local_envvar(c(TESTTHAT_PARALLEL = "TRUE"))
-  withr::defer(unlink(tmp, recursive = TRUE))
-  dir.create(tmp <- tempfile("testthat-snap-"))
+  tmp <- withr::local_tempdir("testthat-snap-")
   file.copy(test_path("test-parallel", "snap"), tmp, recursive = TRUE)
   # we cannot run these with the silent reporter, because it is not
   # parallel compatible, and they'll not run in parallel
@@ -82,11 +81,11 @@ test_that("snapshots", {
 })
 
 test_that("new snapshots are added", {
-  withr::local_envvar(c(TESTTHAT_PARALLEL = "TRUE"))
-  withr::defer(unlink(tmp, recursive = TRUE))
-  dir.create(tmp <- tempfile("testthat-snap-"))
+  withr::local_envvar(c(TESTTHAT_PARALLEL = "TRUE", CI = "false"))
+  tmp <- withr::local_tempdir("testthat-snap-")
   file.copy(test_path("test-parallel", "snap"), tmp, recursive = TRUE)
   unlink(file.path(tmp, "snap", "tests", "testthat", "_snaps", "snap-2.md"))
+
   # we cannot run these with the silent reporter, because it is not
   # parallel compatible, and they'll not run in parallel
   capture.output(suppressMessages(
@@ -107,13 +106,13 @@ test_that("new snapshots are added", {
 
 test_that("snapshots are removed if test file has no snapshots", {
   withr::local_envvar(c(TESTTHAT_PARALLEL = "TRUE"))
-  withr::defer(unlink(tmp, recursive = TRUE))
-  dir.create(tmp <- tempfile("testthat-snap-"))
+  tmp <- withr::local_tempdir("testthat-snap-")
   file.copy(test_path("test-parallel", "snap"), tmp, recursive = TRUE)
   writeLines(
     "test_that(\"2\", { expect_true(TRUE) })",
     file.path(tmp, "snap", "tests", "testthat", "test-snap-2.R")
   )
+
   # we cannot run these with the silent reporter, because it is not
   # parallel compatible, and they'll not run in parallel
   capture.output(suppressMessages(