From 5a8200a001dc9329c507a6913a83151d8c1c6fac Mon Sep 17 00:00:00 2001
From: Hadley Wickham <h.wickham@gmail.com>
Date: Wed, 6 Nov 2024 10:01:19 -0600
Subject: [PATCH] Improve ... checking logic for 3e `expect_error()` (and
 friends) (#2014)

Fixes #1932
---
 NEWS.md                                   |  1 +
 R/expect-condition.R                      | 24 +++++++++++++++++------
 tests/testthat/_snaps/expect-condition.md | 16 +++++++++------
 tests/testthat/test-expect-condition.R    |  7 +++++--
 4 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/NEWS.md b/NEWS.md
index c1877aff1..c9028b2b7 100644
--- a/NEWS.md
+++ b/NEWS.md
@@ -1,5 +1,6 @@
 # testthat (development version)
 
+* `expect_error()` and friends now error if you supply `...` but not `pattern` (#1932).
 * New `expect_no_failure()`, `expect_no_success()` and `expect_snapshot_failure()` provide more options for testing expectations.
 * `expect_error()` and friends no longer give an uninformative error if they fail inside a magrittr pipe (#1994).
 * `expect_setequal()` correctly identifies what is missing where (#1962).
diff --git a/R/expect-condition.R b/R/expect-condition.R
index ce5c1d4a6..f032b5573 100644
--- a/R/expect-condition.R
+++ b/R/expect-condition.R
@@ -253,11 +253,6 @@ expect_condition_matching <- function(base_class,
                                       label = NULL,
                                       trace_env = caller_env(),
                                       error_call = caller_env()) {
-
-  check_dots_used(error = function(cnd) {
-    warn(conditionMessage(cnd), call = error_call)
-  })
-
   matcher <- cnd_matcher(
     base_class,
     class,
@@ -299,6 +294,13 @@ cnd_matcher <- function(base_class,
   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
+    )
+  }
+
   function(cnd) {
     if (!inherit) {
       cnd$parent <- NULL
@@ -316,7 +318,17 @@ cnd_matcher <- function(base_class,
         return(FALSE)
       }
       if (!is.null(pattern) && !isNA(pattern)) {
-        grepl(pattern, conditionMessage(x), ...)
+        withCallingHandlers(
+          grepl(pattern, conditionMessage(x), ...),
+          error = function(e) {
+            cli::cli_abort(
+              "Failed to compare {base_class} to {.arg pattern}.",
+              parent = e,
+              call = error_call
+            )
+          }
+        )
+
       } else {
         TRUE
       }
diff --git a/tests/testthat/_snaps/expect-condition.md b/tests/testthat/_snaps/expect-condition.md
index 7afc7df7d..c46f356ac 100644
--- a/tests/testthat/_snaps/expect-condition.md
+++ b/tests/testthat/_snaps/expect-condition.md
@@ -29,14 +29,18 @@
 
     `f1()` did not throw a condition with class <bar>.
 
-# unused arguments generate a warning
+# unused arguments generate an error
 
     Code
       expect_condition(stop("Hi!"), foo = "bar")
     Condition
-      Warning in `expect_condition()`:
-      Arguments in `...` must be used.
-      x Problematic argument:
-      * foo = "bar"
-      i Did you misspell an argument name?
+      Error in `expect_condition()`:
+      ! Can't specify `...` without `pattern`.
+    Code
+      expect_condition(stop("Hi!"), "x", foo = "bar")
+    Condition
+      Error in `expect_condition()`:
+      ! Failed to compare condition to `pattern`.
+      Caused by error in `grepl()`:
+      ! unused argument (foo = "bar")
 
diff --git a/tests/testthat/test-expect-condition.R b/tests/testthat/test-expect-condition.R
index 4ee1a9234..33e3d1711 100644
--- a/tests/testthat/test-expect-condition.R
+++ b/tests/testthat/test-expect-condition.R
@@ -223,8 +223,11 @@ test_that("can match parent conditions (#1493)", {
   expect_error(expect_error(f(), "Parent message.", inherit = FALSE))
 })
 
-test_that("unused arguments generate a warning", {
-  expect_snapshot(expect_condition(stop("Hi!"), foo = "bar"))
+test_that("unused arguments generate an error", {
+  expect_snapshot(error = TRUE, {
+    expect_condition(stop("Hi!"), foo = "bar")
+    expect_condition(stop("Hi!"), "x", foo = "bar")
+  })
 })