From 8eb147ba770a1e0b9f9254d417f4caece57ee052 Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Tue, 5 Nov 2024 13:15:38 -0600 Subject: [PATCH 1/3] Improve ... checking logic for 3e `expect_error()` (and friends) Fixes #1932 --- NEWS.md | 1 + R/expect-condition.R | 23 ++++++++++++++++++----- tests/testthat/_snaps/expect-condition.md | 16 ++++++++++------ tests/testthat/test-expect-condition.R | 7 +++++-- 4 files changed, 34 insertions(+), 13 deletions(-) diff --git a/NEWS.md b/NEWS.md index 1f595bb41..9bc6670e6 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). * `expect_true()` and `expect_false()` give better errors if `actual` isn't a vector (#1996). * `expect_no_*()` expectations no longer incorrectly emit a passing test result if they in fact fail (#1997). * Require the latest version of waldo (0.6.0) in order to get the latest goodies (#1955). diff --git a/R/expect-condition.R b/R/expect-condition.R index 6f299cc59..dbbcb62b2 100644 --- a/R/expect-condition.R +++ b/R/expect-condition.R @@ -256,10 +256,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, @@ -301,6 +297,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( + "{.arg ...} ignored when {.arg pattern} is not set.", + call = error_call + ) + } + function(cnd) { if (!inherit) { cnd$parent <- NULL @@ -318,7 +321,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 message 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..a39d112a9 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 . -# 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: + ! `...` ignored when `pattern` is not set. + Code + expect_condition(stop("Hi!"), "x", foo = "bar") + Condition + Error: + ! Failed to compare message 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 d5cff5b02..53d892862 100644 --- a/tests/testthat/test-expect-condition.R +++ b/tests/testthat/test-expect-condition.R @@ -217,8 +217,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") + }) }) From d3c16cd07bbbee63b99f5f699281563e8e0fa05f Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 6 Nov 2024 09:45:54 -0600 Subject: [PATCH 2/3] Tweak error --- R/expect-condition.R | 2 +- tests/testthat/_snaps/expect-condition.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/expect-condition.R b/R/expect-condition.R index 3381407f4..741548d54 100644 --- a/R/expect-condition.R +++ b/R/expect-condition.R @@ -296,7 +296,7 @@ cnd_matcher <- function(base_class, if (is.null(pattern) && dots_n(...) > 0) { cli::cli_abort( - "{.arg ...} ignored when {.arg pattern} is not set.", + "Can't specify {.arg ...} without {.arg pattern}.", call = error_call ) } diff --git a/tests/testthat/_snaps/expect-condition.md b/tests/testthat/_snaps/expect-condition.md index f74a1d8fa..0763dce94 100644 --- a/tests/testthat/_snaps/expect-condition.md +++ b/tests/testthat/_snaps/expect-condition.md @@ -35,7 +35,7 @@ expect_condition(stop("Hi!"), foo = "bar") Condition Error in `expect_condition()`: - ! `...` ignored when `pattern` is not set. + ! Can't specify `...` without `pattern`. Code expect_condition(stop("Hi!"), "x", foo = "bar") Condition From acc5cf8a75640272eae6492192a9b4ee7b95bdbe Mon Sep 17 00:00:00 2001 From: Hadley Wickham Date: Wed, 6 Nov 2024 09:48:57 -0600 Subject: [PATCH 3/3] Tweak error wrapper --- R/expect-condition.R | 2 +- tests/testthat/_snaps/expect-condition.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/expect-condition.R b/R/expect-condition.R index 741548d54..f032b5573 100644 --- a/R/expect-condition.R +++ b/R/expect-condition.R @@ -322,7 +322,7 @@ cnd_matcher <- function(base_class, grepl(pattern, conditionMessage(x), ...), error = function(e) { cli::cli_abort( - "Failed to compare message to {.arg pattern}.", + "Failed to compare {base_class} to {.arg pattern}.", parent = e, call = error_call ) diff --git a/tests/testthat/_snaps/expect-condition.md b/tests/testthat/_snaps/expect-condition.md index 0763dce94..c46f356ac 100644 --- a/tests/testthat/_snaps/expect-condition.md +++ b/tests/testthat/_snaps/expect-condition.md @@ -40,7 +40,7 @@ expect_condition(stop("Hi!"), "x", foo = "bar") Condition Error in `expect_condition()`: - ! Failed to compare message to `pattern`. + ! Failed to compare condition to `pattern`. Caused by error in `grepl()`: ! unused argument (foo = "bar")