From 0746701f6b62b96f605a0ef7b4b1e3472ae1721b Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Mon, 28 Jul 2025 14:51:58 -0700 Subject: [PATCH 01/11] add special case handling in expect_named when ignore.order = TRUE --- R/expect-named.R | 45 ++++++++++++++++++++------- tests/testthat/_snaps/expect-named.md | 19 +++++++++++ tests/testthat/test-expect-named.R | 12 +++++++ 3 files changed, 65 insertions(+), 11 deletions(-) create mode 100644 tests/testthat/_snaps/expect-named.md diff --git a/R/expect-named.R b/R/expect-named.R index 0c90c56f1..611d6624c 100644 --- a/R/expect-named.R +++ b/R/expect-named.R @@ -41,19 +41,42 @@ expect_named <- function( sprintf("%s does not have names.", act$lab) ) } else { - exp_names <- normalise_names(expected, ignore.order, ignore.case) + exp <- quasi_label(enquo(expected), arg = "expected") + exp$val <- normalise_names(exp$val, ignore.order, ignore.case) act$names <- normalise_names(act$names, ignore.order, ignore.case) - expect( - identical(act$names, exp_names), - sprintf( - "Names of %s (%s) don't match %s", - act$lab, - paste0("'", act$names, "'", collapse = ", "), - paste0("'", exp_names, "'", collapse = ", ") - ), - info = info - ) + if (ignore.order) { + act_miss <- unique(act$names[!act$names %in% exp$val]) + exp_miss <- unique(exp$val[!exp$val %in% act$names]) + + expect( + length(exp_miss) == 0 && length(act_miss) == 0, + paste0( + "Names of ", + act$lab, + " (`actual`) and ", + exp$lab, + " (`expected`) don't have the same values.\n", + if (length(act_miss)) { + paste0("* Only in `actual`: ", values(act_miss), "\n") + }, + if (length(exp_miss)) { + paste0("* Only in `expected`: ", values(exp_miss), "\n") + } + ) + ) + } else { + expect( + identical(act$names, exp$val), + sprintf( + "Names of %s (%s) don't match %s", + act$lab, + paste0("'", act$names, "'", collapse = ", "), + paste0("'", exp$val, "'", collapse = ", ") + ), + info = info + ) + } } invisible(act$val) } diff --git a/tests/testthat/_snaps/expect-named.md b/tests/testthat/_snaps/expect-named.md new file mode 100644 index 000000000..90ab47662 --- /dev/null +++ b/tests/testthat/_snaps/expect-named.md @@ -0,0 +1,19 @@ +# provide useful feedback on failure + + Names of c(a = 1) (`actual`) and c("a", "b") (`expected`) don't have the same values. + * Only in `expected`: "b" + + +--- + + Names of c(a = 1, b = 1) (`actual`) and c("a") (`expected`) don't have the same values. + * Only in `actual`: "b" + + +--- + + Names of c(a = 1) (`actual`) and c("b") (`expected`) don't have the same values. + * Only in `actual`: "a" + * Only in `expected`: "b" + + diff --git a/tests/testthat/test-expect-named.R b/tests/testthat/test-expect-named.R index 68da9a63d..287eb23d2 100644 --- a/tests/testthat/test-expect-named.R +++ b/tests/testthat/test-expect-named.R @@ -19,3 +19,15 @@ test_that("expected_named optionally ignores order", { ignore.order = TRUE )) }) + +test_that("provide useful feedback on failure", { + expect_snapshot_error( + expect_named(c(a = 1), c("a", "b"), ignore.order = TRUE) + ) + expect_snapshot_error( + expect_named(c(a = 1, b = 1), c("a"), ignore.order = TRUE) + ) + expect_snapshot_error( + expect_named(c(a = 1), c("b"), ignore.order = TRUE) + ) +}) From 78f4a4ad3efe37157ab5775a1bbdc77877f2f0ef Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Wed, 30 Jul 2025 20:34:15 -0700 Subject: [PATCH 02/11] switch expect_named() from using expect() to pass()/fail() --- R/expect-named.R | 67 +++++++++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 32 deletions(-) diff --git a/R/expect-named.R b/R/expect-named.R index 67b01d859..f883f0716 100644 --- a/R/expect-named.R +++ b/R/expect-named.R @@ -39,45 +39,48 @@ expect_named <- function( if (identical(act$names, NULL)) { msg <- sprintf("%s does not have names.", act$lab) return(fail(msg)) + } else { + return(pass(act$val)) } - } else { - exp <- quasi_label(enquo(expected), arg = "expected") - exp$val <- normalise_names(exp$val, ignore.order, ignore.case) - act$names <- normalise_names(act$names, ignore.order, ignore.case) + } - if (ignore.order) { - act_miss <- unique(act$names[!act$names %in% exp$val]) - exp_miss <- unique(exp$val[!exp$val %in% act$names]) + exp <- quasi_label(enquo(expected), arg = "expected") + + exp$val <- normalise_names(exp$val, ignore.order, ignore.case) + act$names <- normalise_names(act$names, ignore.order, ignore.case) + + if (ignore.order) { + act_miss <- unique(act$names[!act$names %in% exp$val]) + exp_miss <- unique(exp$val[!exp$val %in% act$names]) - expect( - length(exp_miss) == 0 && length(act_miss) == 0, - paste0( - "Names of ", - act$lab, - " (`actual`) and ", - exp$lab, - " (`expected`) don't have the same values.\n", - if (length(act_miss)) { - paste0("* Only in `actual`: ", values(act_miss), "\n") - }, - if (length(exp_miss)) { - paste0("* Only in `expected`: ", values(exp_miss), "\n") - } - ) + if (length(exp_miss) != 0 || length(act_miss) != 0) { + msg <- paste0( + "Names of ", + act$lab, + " (`actual`) and ", + exp$lab, + " (`expected`) don't have the same values.\n", + if (length(act_miss)) { + paste0("* Only in `actual`: ", values(act_miss), "\n") + }, + if (length(exp_miss)) { + paste0("* Only in `expected`: ", values(exp_miss), "\n") + } ) - } else { - expect( - identical(act$names, exp$val), - sprintf( - "Names of %s (%s) don't match %s", - act$lab, - paste0("'", act$names, "'", collapse = ", "), - paste0("'", exp$val, "'", collapse = ", ") - ), - info = info + return(fail(msg)) + } + } else { + if (!identical(act$names, exp$val)) { + msg <- sprintf( + "Names of %s (%s) don't match %s", + act$lab, + paste0("'", act$names, "'", collapse = ", "), + paste0("'", exp$val, "'", collapse = ", ") ) + return(fail(msg)) } } + pass(act$val) } From 594be56975505814cd981e9b5e138c009e7bb34e Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:22:33 -0700 Subject: [PATCH 03/11] extract out expect_has_names_() --- R/expect-named.R | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/R/expect-named.R b/R/expect-named.R index 8831ee507..d74867efa 100644 --- a/R/expect-named.R +++ b/R/expect-named.R @@ -39,12 +39,7 @@ expect_named <- function( act$names <- names(act$val) if (missing(expected)) { - if (identical(act$names, NULL)) { - msg <- sprintf("%s does not have names.", act$lab) - return(fail(msg)) - } else { - return(pass(act$val)) - } + return(expect_has_names_(act)) } exp <- quasi_label(enquo(expected), arg = "expected") @@ -101,3 +96,11 @@ normalise_names <- function(x, ignore.order = FALSE, ignore.case = FALSE) { x } + +expect_has_names_ <- function(act) { + if (identical(act$names, NULL)) { + msg <- sprintf("%s does not have names.", act$lab) + return(fail(msg)) + } + return(pass(act$val)) +} From 8f59d3456154553dc7314093257b0d21b954b1bb Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:33:16 -0700 Subject: [PATCH 04/11] refactor out expect_setequal_() --- R/expect-setequal.R | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/R/expect-setequal.R b/R/expect-setequal.R index 02f7cb4e8..0b6c0d04d 100644 --- a/R/expect-setequal.R +++ b/R/expect-setequal.R @@ -33,11 +33,15 @@ expect_setequal <- function(object, expected) { testthat_warn("expect_setequal() ignores names") } + expect_setequal_(act, exp) +} + +expect_setequal_ <- function(act, exp, trace_env = caller_env()) { act_miss <- unique(act$val[!act$val %in% exp$val]) exp_miss <- unique(exp$val[!exp$val %in% act$val]) if (length(exp_miss) || length(act_miss)) { - return(fail(paste0( + msg <- paste0( act$lab, " (`actual`) and ", exp$lab, @@ -48,7 +52,8 @@ expect_setequal <- function(object, expected) { if (length(exp_miss)) { paste0("* Only in `expected`: ", values(exp_miss), "\n") } - ))) + ) + return(fail(msg, trace_env = trace_env)) } pass(act$val) } From cb09657e6ba0431a0d8cecf332dbae57dfb200a1 Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:33:27 -0700 Subject: [PATCH 05/11] use expect_setequal_() in expect_named() --- R/expect-named.R | 23 +++-------------------- tests/testthat/_snaps/expect-named.md | 6 +++--- 2 files changed, 6 insertions(+), 23 deletions(-) diff --git a/R/expect-named.R b/R/expect-named.R index d74867efa..5b7151a23 100644 --- a/R/expect-named.R +++ b/R/expect-named.R @@ -45,28 +45,11 @@ expect_named <- function( exp <- quasi_label(enquo(expected), arg = "expected") exp$val <- normalise_names(exp$val, ignore.order, ignore.case) - act$names <- normalise_names(act$names, ignore.order, ignore.case) + act_names <- normalise_names(act$names, ignore.order, ignore.case) if (ignore.order) { - act_miss <- unique(act$names[!act$names %in% exp$val]) - exp_miss <- unique(exp$val[!exp$val %in% act$names]) - - if (length(exp_miss) != 0 || length(act_miss) != 0) { - msg <- paste0( - "Names of ", - act$lab, - " (`actual`) and ", - exp$lab, - " (`expected`) don't have the same values.\n", - if (length(act_miss)) { - paste0("* Only in `actual`: ", values(act_miss), "\n") - }, - if (length(exp_miss)) { - paste0("* Only in `expected`: ", values(exp_miss), "\n") - } - ) - return(fail(msg)) - } + act <- labelled_value(act_names, act$lab) + return(expect_setequal_(act, exp)) } else { if (!identical(act$names, exp$val)) { msg <- sprintf( diff --git a/tests/testthat/_snaps/expect-named.md b/tests/testthat/_snaps/expect-named.md index 9470fbc54..a830f9004 100644 --- a/tests/testthat/_snaps/expect-named.md +++ b/tests/testthat/_snaps/expect-named.md @@ -1,18 +1,18 @@ # provide useful feedback on failure - Names of c(a = 1) (`actual`) and c("a", "b") (`expected`) don't have the same values. + c(a = 1) (`actual`) and c("a", "b") (`expected`) don't have the same values. * Only in `expected`: "b" --- - Names of c(a = 1, b = 1) (`actual`) and c("a") (`expected`) don't have the same values. + c(a = 1, b = 1) (`actual`) and c("a") (`expected`) don't have the same values. * Only in `actual`: "b" --- - Names of c(a = 1) (`actual`) and c("b") (`expected`) don't have the same values. + c(a = 1) (`actual`) and c("b") (`expected`) don't have the same values. * Only in `actual`: "a" * Only in `expected`: "b" From 13cc3e2ad370c6719e3d9469db8062c4a62aff2a Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:38:57 -0700 Subject: [PATCH 06/11] use expect_waldo_equal_() in expect_named() --- R/expect-named.R | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/R/expect-named.R b/R/expect-named.R index 5b7151a23..b83943f6f 100644 --- a/R/expect-named.R +++ b/R/expect-named.R @@ -49,17 +49,10 @@ expect_named <- function( if (ignore.order) { act <- labelled_value(act_names, act$lab) - return(expect_setequal_(act, exp)) + return(expect_setequal_(act, exp, prefix = "Names of ")) } else { - if (!identical(act$names, exp$val)) { - msg <- sprintf( - "Names of %s (%s) don't match %s", - act$lab, - paste0("'", act$names, "'", collapse = ", "), - paste0("'", exp$val, "'", collapse = ", ") - ) - return(fail(msg)) - } + act <- labelled_value(act_names, act$lab) + return(expect_waldo_equal_("equal", act, exp)) } pass(act$val) From b29cd069939f31ab5b32251501393c51fbdb7639 Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:43:17 -0700 Subject: [PATCH 07/11] add error_prefix to expect_setequal_() and expect_waldo_equal_() --- R/expect-equality.R | 6 +++++- R/expect-named.R | 4 ++-- R/expect-setequal.R | 10 +++++++++- tests/testthat/_snaps/expect-named.md | 27 ++++++++++++++++++++++++--- tests/testthat/test-expect-named.R | 10 ++++++++++ 5 files changed, 50 insertions(+), 7 deletions(-) diff --git a/R/expect-equality.R b/R/expect-equality.R index 399f98f15..82b0999d0 100644 --- a/R/expect-equality.R +++ b/R/expect-equality.R @@ -126,7 +126,8 @@ expect_waldo_equal_ <- function( exp, info = NULL, ..., - trace_env = caller_env() + trace_env = caller_env(), + error_prefix = NULL ) { comp <- waldo_compare( act$val, @@ -145,6 +146,9 @@ expect_waldo_equal_ <- function( "`expected`", paste0(comp, collapse = "\n\n") ) + if (!is.null(error_prefix)) { + msg <- paste0(error_prefix, msg) + } return(fail(msg, info = info, trace_env = trace_env)) } pass(act$val) diff --git a/R/expect-named.R b/R/expect-named.R index b83943f6f..ba1e4b1bb 100644 --- a/R/expect-named.R +++ b/R/expect-named.R @@ -49,10 +49,10 @@ expect_named <- function( if (ignore.order) { act <- labelled_value(act_names, act$lab) - return(expect_setequal_(act, exp, prefix = "Names of ")) + return(expect_setequal_(act, exp, error_prefix = "Names of ")) } else { act <- labelled_value(act_names, act$lab) - return(expect_waldo_equal_("equal", act, exp)) + return(expect_waldo_equal_("equal", act, exp, error_prefix = "Names of ")) } pass(act$val) diff --git a/R/expect-setequal.R b/R/expect-setequal.R index 0b6c0d04d..9952dd5f6 100644 --- a/R/expect-setequal.R +++ b/R/expect-setequal.R @@ -36,12 +36,20 @@ expect_setequal <- function(object, expected) { expect_setequal_(act, exp) } -expect_setequal_ <- function(act, exp, trace_env = caller_env()) { +expect_setequal_ <- function( + act, + exp, + trace_env = caller_env(), + error_prefix = NULL +) { act_miss <- unique(act$val[!act$val %in% exp$val]) exp_miss <- unique(exp$val[!exp$val %in% act$val]) if (length(exp_miss) || length(act_miss)) { msg <- paste0( + if (!is.null(error_prefix)) { + error_prefix + }, act$lab, " (`actual`) and ", exp$lab, diff --git a/tests/testthat/_snaps/expect-named.md b/tests/testthat/_snaps/expect-named.md index a830f9004..3a54b0219 100644 --- a/tests/testthat/_snaps/expect-named.md +++ b/tests/testthat/_snaps/expect-named.md @@ -1,22 +1,43 @@ # provide useful feedback on failure - c(a = 1) (`actual`) and c("a", "b") (`expected`) don't have the same values. + Names of c(a = 1) (`actual`) and c("a", "b") (`expected`) don't have the same values. * Only in `expected`: "b" --- - c(a = 1, b = 1) (`actual`) and c("a") (`expected`) don't have the same values. + Names of c(a = 1, b = 1) (`actual`) and c("a") (`expected`) don't have the same values. * Only in `actual`: "b" --- - c(a = 1) (`actual`) and c("b") (`expected`) don't have the same values. + Names of c(a = 1) (`actual`) and c("b") (`expected`) don't have the same values. * Only in `actual`: "a" * Only in `expected`: "b" +--- + + Names of c(a = 1) (`actual`) is not equal to c("a", "b") (`expected`). + + `actual`: "a" + `expected`: "a" "b" + +--- + + Names of c(a = 1, b = 1) (`actual`) is not equal to c("a") (`expected`). + + `actual`: "a" "b" + `expected`: "a" + +--- + + Names of c(a = 1) (`actual`) is not equal to c("b") (`expected`). + + `actual`: "a" + `expected`: "b" + # expect_named validates its inputs Code diff --git a/tests/testthat/test-expect-named.R b/tests/testthat/test-expect-named.R index f86732707..347d1a734 100644 --- a/tests/testthat/test-expect-named.R +++ b/tests/testthat/test-expect-named.R @@ -30,6 +30,16 @@ test_that("provide useful feedback on failure", { expect_snapshot_error( expect_named(c(a = 1), c("b"), ignore.order = TRUE) ) + + expect_snapshot_error( + expect_named(c(a = 1), c("a", "b"), ignore.order = FALSE) + ) + expect_snapshot_error( + expect_named(c(a = 1, b = 1), c("a"), ignore.order = FALSE) + ) + expect_snapshot_error( + expect_named(c(a = 1), c("b"), ignore.order = FALSE) + ) }) test_that("expect_named validates its inputs", { From e9e5812370ce408c619519e7226e7825bae02e6d Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:44:10 -0700 Subject: [PATCH 08/11] remove redundency in act_names --- R/expect-named.R | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/R/expect-named.R b/R/expect-named.R index ba1e4b1bb..842abb88c 100644 --- a/R/expect-named.R +++ b/R/expect-named.R @@ -36,7 +36,6 @@ expect_named <- function( check_bool(ignore.case) act <- quasi_label(enquo(object), label) - act$names <- names(act$val) if (missing(expected)) { return(expect_has_names_(act)) @@ -45,7 +44,7 @@ expect_named <- function( exp <- quasi_label(enquo(expected), arg = "expected") exp$val <- normalise_names(exp$val, ignore.order, ignore.case) - act_names <- normalise_names(act$names, ignore.order, ignore.case) + act_names <- normalise_names(names(act$val), ignore.order, ignore.case) if (ignore.order) { act <- labelled_value(act_names, act$lab) @@ -74,7 +73,8 @@ normalise_names <- function(x, ignore.order = FALSE, ignore.case = FALSE) { } expect_has_names_ <- function(act) { - if (identical(act$names, NULL)) { + act_names <- names(act$val) + if (identical(act_names, NULL)) { msg <- sprintf("%s does not have names.", act$lab) return(fail(msg)) } From 81e4ee82edb14e3357a642075e6b9efda1d2dc21 Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:46:29 -0700 Subject: [PATCH 09/11] add news --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index fb3221d5a..bac71b7a7 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ # testthat (development version) +* `expect_named()` now gives more informative errors (#2091). * `expect_*()` functions consistently and rigorously check their inputs (#1754). * `JunitReporter()` no longer fails with `"no applicable method for xml_add_child"` for warnings outside of tests (#1913). Additionally, warnings now save their backtraces. * `JunitReporter()` strips ANSI escapes in more placese (#1852, #2032). From 22bf125addbcfab9f861e753204126ec5f3b1818 Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:48:12 -0700 Subject: [PATCH 10/11] add test for glue strings in expect_named() --- tests/testthat/test-expect-named.R | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/testthat/test-expect-named.R b/tests/testthat/test-expect-named.R index 347d1a734..c25bedc69 100644 --- a/tests/testthat/test-expect-named.R +++ b/tests/testthat/test-expect-named.R @@ -48,3 +48,13 @@ test_that("expect_named validates its inputs", { expect_named(c(a = 1), "a", ignore.case = "yes") }) }) + +test_that("expect_named accepts glue for 'expected'", { + n <- structure( + c("v1", "v2", "v3", "v4", "v5"), + class = c("glue", "character") + ) + v <- set_names(1:5, n) + + expect_named(v, n) +}) From ea71d8ad3e139bb0cd92ce3203e348313c37557d Mon Sep 17 00:00:00 2001 From: Emil Hvitfeldt Date: Fri, 1 Aug 2025 16:53:34 -0700 Subject: [PATCH 11/11] add missing trace_env arg in expect_nas_names_() --- R/expect-named.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/expect-named.R b/R/expect-named.R index 842abb88c..28cd96f6e 100644 --- a/R/expect-named.R +++ b/R/expect-named.R @@ -72,11 +72,11 @@ normalise_names <- function(x, ignore.order = FALSE, ignore.case = FALSE) { x } -expect_has_names_ <- function(act) { +expect_has_names_ <- function(act, trace_env = caller_env()) { act_names <- names(act$val) if (identical(act_names, NULL)) { msg <- sprintf("%s does not have names.", act$lab) - return(fail(msg)) + return(fail(msg, trace_env = trace_env)) } return(pass(act$val)) }