Actually fail with new snapshots on CI

By defaulting `fail_on_new` to `NULL` in more

Continuation of #2149. Fixes #2176.

Author: hadley

R/parallel.R

@@ -88,7 +88,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
@@ -303,10 +303,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

@@ -194,12 +194,13 @@ snapshot_file_equal <- function(
   snap_variant,
   path,
   file_equal = compare_file_binary,
-  fail_on_new = FALSE,
+  fail_on_new = NULL,
   trace_env = caller_env()
 ) {
   if (!file.exists(path)) {
     abort(paste0("`", path, "` not found"))
   }
+  fail_on_new <- fail_on_new %||% on_ci()
 
   cur_path <- file.path(snap_test_dir, snap_name)
   new_path <- new_name(cur_path)
@@ -224,7 +225,10 @@ snapshot_file_equal <- function(
       snap_name,
       "'"
     )
-    if (fail_on_new) {
+
+    # We want to fail on CI since this suggests that the user has failed
+    # to record the value locally
+    if (on_ci()) {
       return(fail(message, trace_env = trace_env))
     } else {
       testthat_warn(message)

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,15 +82,16 @@ 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",
+          if (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)
@@ -200,7 +201,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/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)),