Improve error about ... in expect_condition() (et al) (#2055)

Fixes #2054. Fixes #2053.

Author: MichaelChirico

R/expect-condition.R

@@ -280,6 +280,7 @@ expect_condition_matching <- function(
   trace_env = caller_env(),
   error_call = caller_env()
 ) {
+  check_condition_dots(regexp, ..., error_call = error_call)
   matcher <- cnd_matcher(
     base_class,
     class,
@@ -320,21 +321,14 @@ expect_condition_matching <- function(
 cnd_matcher <- function(
   base_class,
   class = NULL,
-  pattern = NULL,
+  regexp = NULL,
   ...,
   inherit = TRUE,
   ignore_deprecation = FALSE,
   error_call = caller_env()
 ) {
   check_string(class, allow_null = TRUE, call = error_call)
-  check_string(pattern, allow_null = TRUE, allow_na = TRUE, call = error_call)
-
-  if (is.null(pattern) && dots_n(...) > 0) {
-    cli::cli_abort(
-      "Can't specify {.arg ...} without {.arg pattern}.",
-      call = error_call
-    )
-  }
+  check_string(regexp, allow_null = TRUE, allow_na = TRUE, call = error_call)
 
   function(cnd) {
     if (!inherit) {
@@ -352,12 +346,12 @@ cnd_matcher <- function(
       if (!is.null(class) && !inherits(x, class)) {
         return(FALSE)
       }
-      if (!is.null(pattern) && !isNA(pattern)) {
+      if (!is.null(regexp) && !isNA(regexp)) {
         withCallingHandlers(
-          grepl(pattern, conditionMessage(x), ...),
+          grepl(regexp, conditionMessage(x), ...),
           error = function(e) {
             cli::cli_abort(
-              "Failed to compare {base_class} to {.arg pattern}.",
+              "Failed to compare {base_class} to {.arg regexp}.",
               parent = e,
               call = error_call
             )
@@ -582,3 +576,29 @@ cnd_message <- function(x) {
   withr::local_options(rlang_backtrace_on_error = "none")
   conditionMessage(x)
 }
+
+check_condition_dots <- function(
+  regexp = NULL,
+  ...,
+  error_call = caller_env()
+) {
+  if (!is.null(regexp) || missing(...)) {
+    return()
+  }
+
+  dot_names <- ...names()
+  if (is.null(dot_names)) {
+    dot_names <- rep("", ...length())
+  }
+  unnamed <- dot_names == ""
+  dot_names[unnamed] <- paste0("..", seq_along(dot_names)[unnamed])
+
+  cli::cli_abort(
+    c(
+      "Can't supply {.arg ...} unless {.arg regexp} is set.",
+      "*" = "Unused arguments: {.arg {dot_names}}.",
+      i = "Did you mean to use {.arg regexp} so {.arg ...} is passed to {.fn grepl}?"
+    ),
+    call = error_call
+  )
+}

R/expect-no-condition.R

@@ -76,7 +76,7 @@ expect_no_ <- function(
   matcher <- cnd_matcher(
     base_class,
     class,
-    pattern = regexp,
+    regexp = regexp,
     ignore_deprecation = base_class == "warning" &&
       is.null(regexp) &&
       is.null(class)

R/test-compiled-code.R

@@ -143,7 +143,11 @@ run_cpp_tests <- function(package) {
           c(line, line, 1, 1)
         )
 
-        exp <- new_expectation("error", exception_text, srcref = exception_srcref)
+        exp <- new_expectation(
+          "error",
+          exception_text,
+          srcref = exception_srcref
+        )
         exp$test <- test_name
 
         get_reporter()$add_result(

tests/testthat/_snaps/expect-condition.md

@@ -12,7 +12,7 @@
       expect_error(stop("!"), regexp = 1)
     Condition
       Error in `expect_error()`:
-      ! `pattern` must be a single string, `NA`, or `NULL`, not the number 1.
+      ! `regexp` must be a single string, `NA`, or `NULL`, not the number 1.
     Code
       expect_error(stop("!"), class = 1)
     Condition
@@ -35,12 +35,35 @@
       expect_condition(stop("Hi!"), foo = "bar")
     Condition
       Error in `expect_condition()`:
-      ! Can't specify `...` without `pattern`.
+      ! Can't supply `...` unless `regexp` is set.
+      * Unused arguments: `foo`.
+      i Did you mean to use `regexp` so `...` is passed to `grepl()`?
+    Code
+      expect_condition(stop("Hi!"), , , "bar")
+    Condition
+      Error in `expect_condition()`:
+      ! Can't supply `...` unless `regexp` is set.
+      * Unused arguments: `..1`.
+      i Did you mean to use `regexp` so `...` is passed to `grepl()`?
+    Code
+      expect_condition(stop("Hi!"), , , "bar", fixed = TRUE)
+    Condition
+      Error in `expect_condition()`:
+      ! Can't supply `...` unless `regexp` is set.
+      * Unused arguments: `..1` and `fixed`.
+      i Did you mean to use `regexp` so `...` is passed to `grepl()`?
     Code
       expect_condition(stop("Hi!"), "x", foo = "bar")
     Condition
       Error in `expect_condition()`:
-      ! Failed to compare condition to `pattern`.
+      ! Failed to compare condition to `regexp`.
       Caused by error in `grepl()`:
       ! unused argument (foo = "bar")
+    Code
+      expect_condition(stop("Hi!"), pattern = "bar", fixed = TRUE)
+    Condition
+      Error in `expect_condition()`:
+      ! Can't supply `...` unless `regexp` is set.
+      * Unused arguments: `pattern` and `fixed`.
+      i Did you mean to use `regexp` so `...` is passed to `grepl()`?
 

tests/testthat/test-catch.R

@@ -7,4 +7,5 @@ test_that("get_routine() fails when no routine exists", {
   expect_error(get_routine("utils", "no_such_routine"))
 })
 
+skip_if_not_installed("xml2")
 run_cpp_tests("testthat")

tests/testthat/test-compare.R

@@ -196,8 +196,14 @@ test_that("base lengths must be identical", {
 })
 
 test_that("tzones must be identical", {
-  t1 <- ISOdatetime(2016, 2, 29, 12, 13, 14, "EST")
-  t2 <- ISOdatetime(2016, 2, 29, 12, 13, 14, "US/Eastern")
+  # skip on minimal setups
+  tryCatch(
+    {
+      t1 <- ISOdatetime(2016, 2, 29, 12, 13, 14, "EST")
+      t2 <- ISOdatetime(2016, 2, 29, 12, 13, 14, "US/Eastern")
+    },
+    warning = function(w) skip(conditionMessage(w))
+  )
 
   expect_match(compare(t1, t2)$message, '"tzone": 1 string mismatch')
 })

tests/testthat/test-expect-condition.R

@@ -252,7 +252,10 @@ test_that("can match parent conditions (#1493)", {
 test_that("unused arguments generate an error", {
   expect_snapshot(error = TRUE, {
     expect_condition(stop("Hi!"), foo = "bar")
+    expect_condition(stop("Hi!"), , , "bar")
+    expect_condition(stop("Hi!"), , , "bar", fixed = TRUE)
     expect_condition(stop("Hi!"), "x", foo = "bar")
+    expect_condition(stop("Hi!"), pattern = "bar", fixed = TRUE)
   })
 })
 

tests/testthat/test-expect-inheritance.R

@@ -75,6 +75,7 @@ test_that("checks its inputs", {
 })
 
 test_that("can check with actual class", {
+  skip_if_not_installed("S7")
   Foo <- S7::new_class("Foo", package = NULL)
   Bar <- S7::new_class("Bar", package = NULL)
   expect_success(expect_s7_class(Foo(), class = Foo))

tests/testthat/test-reporter-junit.R

@@ -1,4 +1,5 @@
 test_that("reporter doesn't change without warning", {
+  skip_if_not_installed("xml2")
   expect_snapshot_reporter(JunitReporterMock$new())
 })